Only run imagemin task inside precommit task if images are added/modified

[iseulde]

Right now it runs all the time.

[iseulde]

Any objections?

[DrewAPicture]

Replying to iseulde:

Any objections?

None here. @jorbin?

[netweb]

I've two use cases where this will trip us up:

1. When core updates grunt-contrib-imagemin without the files being modified they won't benefit from further enhancements of the module.

2. 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 the node_modules folder to run npm install again, or runs npm update, so once again without the files being modified they won't benefit from further enhancements of the module.

[iseulde]

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.

[iseulde]

Also, we could look if there's a change in package.json and in that case run all the tasks.

[netweb]

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 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.

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.

[jorbin]

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.

[iseulde]

In 37211:

Build/Test Tools: Run image tasks only if there are changes

See #36528.

[iseulde]

Shall we run these if package.json is changed, or straight run prerelease if it is changed?

[jorbin]

Replying to iseulde:

Shall we run these if package.json is changed, or straight run prerelease 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.

[iseulde]

@jorbin, something like this?

[jorbin]

In 37650:

Build/Test Tools: Run prerelease as the precommit task for configuration file changes

Whenever package.json or Gruntfile.js is updated, we should assume that it affects everything and run the full monty of tasks.

Fixes #36528.
Props iseulde.

[azaozz]

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.

[azaozz]

In 36528.3.patch:

• Use regex to match in result.stdout.
• Define the callbacks outside of the if statements.

[azaozz]

In 37749:

Grunt: when running precommit use regex to check which files have been modified.

Fixes #36528.