Replace Grunt with wp-scripts
#1541
Conversation
# Conflicts: # package-lock.json # package.json
delawski
added
enhancement
tooling
# Conflicts: # package-lock.json # package.json
src/js/admin-exclude.js
Outdated
There was a problem hiding this comment.
📝 note: I just realized that JS files are incorrectly shown as new files. They have been primarily moved from the ui directory to src. Also, most of them have been modified slightly (import jquery instead of relying on a global object, replace vars with lets and consts, remove IIFEs).
I guess the best (although still no fun) way to see a real diff is to review relevant commits like:
.github/workflows/ci.yml
Outdated
| @@ -51,6 +51,9 @@ jobs: | |||
| - name: Install Composer dependencies | |||
| run: composer install | |||
|
|
|||
| - name: Build | |||
| run: npm run build | |||
There was a problem hiding this comment.
💡 thought
If we lint and test before building then if those fails, it should be slightly shorter.
There was a problem hiding this comment.
I have moved the build step to before testing since - if I remember correctly - I was getting errors and tests were failing. It looks like the PHP unit tests may rely on the build step but I may be wrong and I'll have another look tomorrow.
There was a problem hiding this comment.
I tested it locally and, as expected, we can lint early, before building the assets, however we cannot run PHP Unit tests at that time. It's because now we are using the asset.php files when enqueuing JS files. They are generated as part of the build process. When there is no asset file found, a runtime exception is thrown and tests fail.
In 79b2879 I moved the lint step before the build step.
| /** | ||
| * Main JS file script handle. | ||
| */ | ||
| const SCRIPT_HANDLE = 'wp-stream-alert-highlight-js'; |
There was a problem hiding this comment.
🥜 nitpick
I think that just in case someone is dequeueing this for whatever reason that we should keep the constant or deprecate it?
There was a problem hiding this comment.
I must have missed it – definitely can be removed now :)
I'm sorry, I got it all wrong. You've got a valid point. Since we're not planning to release a major version anytime soon, it'd be wise to deprecate this constant instead of removing it. I'll add it back with a comment.
There was a problem hiding this comment.
Addressed in 5e4acf1.
| $this->plugin->get_version() | ||
| $this->plugin->enqueue_asset( | ||
| 'alerts-list', | ||
| array( 'wp-stream-alerts' ) |
There was a problem hiding this comment.
👏 praise
I love this! So much cleaner.
Fixes #1502.
This PR replaces the legacy Grunt-based build workflow with
@wordpress/scripts-based solution.Along the way, some more general cleanup and reorg have been done:
ui/tosrc/directory.builddirectory by Webpack. They are also enqueued centrally in the plugin.assetsdirectory.var->constorlet) so that the linter is not complaining too much.Checklist
contributing.md).Release Changelog
@wordpress/scripts