Make WordPress Core

Opened 6 years ago

Last modified 3 years ago

#43731 accepted task (blessed)

Use Webpack + NPM scripts to build all the things

Reported by: omarreiss's profile omarreiss Owned by: whyisjake's profile whyisjake
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-refresh
Focuses: Cc:

Description

With the introduction of Webpack in WordPress core, we could consider using it to perform build tasks currently performed with Grunt.

The main advantage of having Webpack build everything is that this would reduce the different tools developers would have to learn in the standard development process for WordPress core. NPM scripts would also make the available tools more discoverable and understandable through package.json.

Having a unified approach towards building / bundling also helps in other area's, such as general asset management and devtooling. It could for example be great if we could start using Webpack dev server at some point to serve assets in development and introduce hot module replacement.

Webpack would be totally suitable for this goal, as can also be read from its documentation:

Webpack is a module bundler. Its main purpose is to bundle JavaScript files for usage in a browser, yet it is also capable of transforming, bundling, or packaging just about any resource or asset.

Webpack provides a unified approach towards building. For example: It would be quite easy to configure Webpack to perform the following tasks:

The above tasks are just some of the more prominent examples. But anything is possible really. NPM scripts can be used to invoke the separate tasks like linting, building etc.

Attachments (9)

jshint.patch (7.9 KB) - added by youknowriad 6 years ago.
JSHint to NPM script
jshint.2.patch (8.1 KB) - added by youknowriad 6 years ago.
I was missing some files in the original patch. That npm script line is way too big, I was thinking of creating a separate bin/jshint script, what do you think?
wordpress_webpack.patch (342.0 KB) - added by herregroen 6 years ago.
wordpress_webpack.2.patch (309.7 KB) - added by herregroen 6 years ago.
wordpress-webpack.patch (73.0 KB) - added by herregroen 5 years ago.
wordpress-webpack.2.patch (73.0 KB) - added by herregroen 5 years ago.
wordpress-webpack.3.patch (74.5 KB) - added by herregroen 5 years ago.
wordpress-webpack.4.patch (74.5 KB) - added by herregroen 5 years ago.
pacman.zip (3.9 MB) - added by jeffreyko3 8 days ago.

Change History (49)

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


6 years ago

#2 @netweb
6 years ago

  • Type changed from enhancement to task (blessed)

#3 @dingo_bastard
6 years ago

I had some issues when using copy-webpack-plugin to bundle my plugin, and I used https://github.com/gregnb/filemanager-webpack-plugin. Worked better than copy-webpack-plugin.

@youknowriad
6 years ago

JSHint to NPM script

#4 @youknowriad
6 years ago

So I started looking at this today, and the Gruntfile.js looks way too complex to move to WebPack on one step. I propose we do it iteratively: move task by task to npm scripts while keeping Grunt as the task manager, and at some point, it will become obvious that we can just remove it.

I know we're also planning to move to ESlint instead of JSHint and this would also be easier if the JSHint setup was a separate npm script. So in this first patch I'm proposing to move the JSHint script to an npm script, still call it in the Gruntfile to keep the toolchain as is (precommit).

I don't have a strong confidence in Trac and the workflow around the patches and stuff. Let me know if I'm doing something wrong.

Last edited 6 years ago by youknowriad (previous) (diff)

@youknowriad
6 years ago

I was missing some files in the original patch. That npm script line is way too big, I was thinking of creating a separate bin/jshint script, what do you think?

This ticket was mentioned in Slack in #core-js by omarreiss. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by davis. View the logs.


6 years ago

#8 @herregroen
6 years ago

During the course of WordCamp Europe I've prepared a patch that will build all source javascript using WebPack.

This allows us to have a single pipeline through which all JS passes. All grunt tasks that were previously involved in building Javascript have been replaced with equivalent WebPack plugins so that everything is handled in one place.

The main advantage of doing so is significantly improved watch times for JS, most files are now under 100ms after these changes on my machine and even the biggest files only take around 350ms.

Overall build times are mostly the same as vendor and node_module dependencies are currently handled in the exact same manner ( copy, minify and validate ). However this does open the path to start importing dependencies such as jQuery through require and building them with the CommonsChunkPlugin, doing so should significantly speed up the overall build time.

Another advantage is that this open up the path to start splitting up some of the extremely large JS files that exist in WordPress, such as the Customiser, into a much more modular approach while maintaining backwards compatibility in our output files. This should make it trivial to set up the Customiser in a similar way as the Media Library.

This will also make it much easier to start extracting useful parts of WordPress JS to the Packages repo and importing these as a dependency into WordPress as well as easily allowing access to the same dependencies for example Gutenberg is using without having to duplicate code or jump through any hoops.

WebPack does add it's own bootstrap code to each file it's build which slightly increases file sizes, while with most files this is an extremely minimal increase we do have some JS files that are only several lines and in these cases it's a relatively large increase. I think the ideal solution here would be to phase out these files and instead use the options WebPack provides to import these where necessary, but if this is problematic on the short term it's possible to adapt the production build to instead of fully building these files simply copy and minify them to ensure there's no difference in the final build.

The current patch successfully builds all JS with no files missing and all tests passing.

#9 @jorbin
6 years ago

@herregroen An initial quick scan through the patch shows a lot of docs changes, including many being deleted and doc blocks being condensed.

#10 @herregroen
6 years ago

@jorbin My bad, I'd based my changes on Omar's working copy of the @output tags which were compressed.

I've uploaded a new patch that's based on the latest trunk.

It is still based on Omar's patch in #44371 as that's required for this to work. His changes there are included in my patch to allow everything to actually work and be testable. Most of the comment changes are from that patch and have simply been moved from the var declaration to the actual assignment.

The only comment change should be removing the @output tag from the embed.js file as this can't be build by webpack due to the issue linked in the comment. I've expanded that comment to explain this.

Last edited 6 years ago by herregroen (previous) (diff)

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by aduth. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by herregroen. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by omarreiss. View the logs.


6 years ago

#15 @mnelson4
6 years ago

I believe using npm and webpack may address https://core.trac.wordpress.org/ticket/44271.

FYI we use npm for our plugin's build process. One significant advantage is that we can use the command webpack --watch --watch-poll (see https://github.com/eventespresso/event-espresso-core/blob/master/docs/AA--Javascript-in-EE/build-process.md#watch) which uses poling instead of waiting for the OS to indicate that a file was changed. This is helpful because normal watching doesn't work when used from inside a VM (like VVV) and your host machine is on Windows.

#16 @omarreiss
6 years ago

In 43577:

General: Explicitly assigns all JS globals to the window.

Many variables in the JavaScript were defined in the global scope without being explicitly assigned to the window. When built with Webpack, the code gets encapsulated in anonymous functions and those implicit globals get assigned to the wrong scope. This patch prevents that from happening.

Fixes #44371. See #43731.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-js by aduth. View the logs.


6 years ago

#19 @pento
5 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Version trunk deleted

This ticket was mentioned in Slack in #core-js by netweb. View the logs.


5 years ago

#21 @herregroen
5 years ago

  • Keywords has-patch added

Patch has been refreshed and seems to be fully functional.

All unit tests pass and using a plain WordPress installations I've not been able to encounter any JS errors or bugs yet.

Watch times on my system seem to average around 100ms. Part of achieving this was making it so all vendor and node_module dependencies aren't being watched. As embed.js needs a unique build process to avoid any ampersands in it it's also not watched. emoji.js is watched but the build that created wp-emoji-release.min.js is not part of the watch task. wp-emoji-release.min.js is also not built as it's a direct concatonation of emoji.js and the twemoji.js library.

As such if we introduce loaders such as babel-loader to use ES6 features these won't be able to be used in that those two files ( emoji.js and embed.js ).

#22 @herregroen
5 years ago

Updated patch to add a comment with the above info to emoji.js ( embed.js already had one ).

Also made sure vendor and node_module dependencies were also build to the src directory when using grunt build --dev.

#23 follow-up: @herregroen
5 years ago

One thing to be aware of is that currently WebPack adds some basic bootstrap to each file build.

As our JS is split across a fair amount of files this does increase the file size of all our JS files, however for minified files this is quite minimal. It adds around 950 characters to each minified JS file.

I think it's possible to separate the bootstrap to a commons file, however this comes with the trade-off that this bootstrap file would need to be additionally loaded on every single page that has JS.

Given the minimal increase I personally feel keeping this simpler is preferable right now.

#24 @herregroen
5 years ago

Patch updated to no longer break the customizer by loading the wrong version of jQuery.

It's now correctly building from the vendor directory instead of the node_modules directory.

This ticket was mentioned in Slack in #core-js by aduth. View the logs.


5 years ago

#26 in reply to: ↑ 23 ; follow-up: @joyously
5 years ago

Replying to herregroen:

I think it's possible to separate the bootstrap to a commons file, however this comes with the trade-off that this bootstrap file would need to be additionally loaded on every single page that has JS.

Given the minimal increase I personally feel keeping this simpler is preferable right now.

How does this affect the JS that plugins get when they use core script handles for dependencies?

#27 in reply to: ↑ 26 @herregroen
5 years ago

Replying to joyously:

Replying to herregroen:

I think it's possible to separate the bootstrap to a commons file, however this comes with the trade-off that this bootstrap file would need to be additionally loaded on every single page that has JS.

Given the minimal increase I personally feel keeping this simpler is preferable right now.

How does this affect the JS that plugins get when they use core script handles for dependencies?

A commons file should itself be listed as a dependency of each core script that relies on it so this would be loaded correctly as long as the core script handles are used.

This ticket was mentioned in Slack in #core-js by herregroen. View the logs.


5 years ago

#29 @omarreiss
5 years ago

  • Milestone changed from Future Release to 5.3

This ticket was mentioned in Slack in #core by joyously. View the logs.


5 years ago

#31 @netweb
5 years ago

  • Milestone changed from 5.3 to 5.4

Due to lack of updates, punting this to 5.4 to clear the way for 5.3

#32 follow-up: @bookdude13
4 years ago

It looks like this task makes #29883 obsolete, could someone who has more experience with the current use of Grunt confirm?

#33 in reply to: ↑ 32 @netweb
4 years ago

Replying to bookdude13:

It looks like this task makes #29883 obsolete, could someone who has more experience with the current use of Grunt confirm?

Done, thanks @bookdude13 👍🏼

#34 follow-up: @davidbaumwald
4 years ago

@netweb Is this still doable in 5.4 with Beta 3 landing tomorrow?

#35 in reply to: ↑ 34 @netweb
4 years ago

  • Milestone changed from 5.4 to 5.5

Replying to davidbaumwald:

@netweb Is this still doable in 5.4 with Beta 3 landing tomorrow?

Same as previous, punting to 5.5, the good thing about the build tools component is much of it doesn't require it to be aligned with any milestone

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#37 @whyisjake
4 years ago

  • Milestone changed from 5.5 to Future Release
  • Owner set to whyisjake
  • Status changed from new to accepted

#38 @iandunn
4 years ago

#49274 might be solved by this effort, if it's taken into consideration (ticket:49274#comment:9).

I think #50412 should also be taken into consideration, since it could be made more or less feasible by what's done here. Ideally a single watcher could trigger PHPUnit tests, since running two watchers causes problems like described in #49274.

#39 @isabel_brison
3 years ago

  • Keywords needs-refresh added

Both the jshint patch and the js build one no longer apply cleanly.

Regarding the jshint one, I think it would be good to a create separate bin/jshint script as per @youknowriad 's comment, and in doing that we should also allow it to take options to lint specific directories or files, as that is already possible with the grunt setup.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 years ago

@jeffreyko3
8 days ago

Note: See TracTickets for help on using tickets.