Opened 9 years ago
Closed 9 years ago
#36528 closed enhancement (fixed)
Only run imagemin task inside precommit task if images are added/modified
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch needs-testing |
Focuses: | Cc: |
Description
Right now it runs all the time.
Attachments (4)
Change History (22)
#3
@
9 years ago
I've two use cases where this will trip us up:
- When core updates
grunt-contrib-imagemin
without the files being modified they won't benefit from further enhancements of the module.
- This is similar to 1, though this time it is due to the nature of NPM's semantic versioning,
grunt-contrib-imagemin
uses"imagemin": "^4.0.0",
hence any further major, minor, or patch level releases in the 4.x branch of Imagemin will be installed by any user who creates a fresh clone/checkout of the develop, or deletes thenode_modules
folder to runnpm install
again, or runsnpm update
, so once again without the files being modified they won't benefit from further enhancements of the module.
#4
follow-up:
↓ 6
@
9 years ago
You'll have exactly the same issue with e.g. the postcss
task for CSS files. I don't think the precommit
task should look at that as you want to make sure your changes are ready for commit or to be uploaded as a patch, and not have all images modified because the module changed. This should be a separate commit and is likely to be the task of someone else, it needs its own ticket etc. At some point someone will run prerelease
which will make sure the changes are detected. We could even make sure all tasks with precommit
are run if there are no file modifications.
#5
@
9 years ago
Also, we could look if there's a change in package.json and in that case run all the tasks.
#6
in reply to:
↑ 4
@
9 years ago
Replying to iseulde:
You'll have exactly the same issue with e.g. the
postcss
task for CSS files.
Indeed, the same issue will happen with PostCSS, and this could be more problematic than the imagemin case as they have more frequent release.
Replying to iseulde:
I don't think the
precommit
task should look at that as you want to make sure your changes are ready for commit or to be uploaded as a patch, and not have all images modified because the module changed. This should be a separate commit and is likely to be the task of someone else, it needs its own ticket etc. At some point someone will runprerelease
which will make sure the changes are detected. We could even make sure all tasks withprecommit
are run if there are no file modifications.
I disagree with this, after each commit grunt build
is ran and these changes are reflected in build.svn.wordpress.org
and many people run /trunk
live on their sites, the primary repo develop.svn.wordpress.org
should be in the most "pristine" condition it can be at all times to reflect the most recent changes for those who run /trunk
.
This should *not* be "someone elses" task ran at "some point", this task should be and I believe is one of the responsibilities of a core committers role.
#7
@
9 years ago
- Keywords commit added
postcss is just the framework. In and of itself, it does nothing. Not sure how that relates to this conversation. Unless you are referring to auto-prefixer which we only really need to worry about every six weeks or so based on browser releases.
I think we are fine relying on grunt release. As the person most likely to commit an update to grunt-contrib-imagemin
, I am also fine running it manually then. Images aren't added often. If someone running trunk is using an image that is a few KB bigger, well I think they are going to be ok.
Patch refreshed as it no longer applied cleanly.
#9
follow-up:
↓ 10
@
9 years ago
Shall we run these if package.json
is changed, or straight run prerelease
?
#10
in reply to:
↑ 9
@
9 years ago
Replying to iseulde:
Shall we run these if
package.json
is changed, or straight runprerelease
if it is changed?
prerelease
seems best to me. Maybe we should also do this if the gruntfile.js changes to be on the safe side? I generally run most of the tasks then anyways just to make sure things are working properly and to see if the build has changed any files.
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
9 years ago
#15
@
9 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
This fails pretty bad on Windows. No matter what is changed, no tasks are run and it exits with Done, without errors.
. Happens because the line breaks in result.stdout
there are \r\n
and the strings don't match.
Perhaps we could strip all \r
before trying to match, but better to use a bit of Regex? It's made for this :)
Also the code can be more readable. Wrapping arrays plus callback functions in if
statements is not. Few inline comments won't hurt either.
Any objections?