WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 2 months ago

Last modified 8 weeks ago

#40894 closed task (blessed) (fixed)

Use Webpack instead of Browserify for build process

Reported by: adamsilverstein Owned by: adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.5
Component: Build/Test Tools Keywords: has-patch commit
Focuses: javascript Cc:

Description

#40834 kicked off a discussion of JavaScript modules in WordPress core.

In a recent JavaScript Core chat the decision was made to move in the direction of using Webpack (and ES6 imports) as our bundler of choice, and to work first on switching out browserfy in our current build chain.

Since we split the media files in #28510, the core build process has used Browserify to combine the media files. While browserify has served us well, Webpack is probably a better long term choice for the project, especially with the introduction of a new JavaScript framework that may require a build.

Attachments (11)

40894.diff (3.9 KB) - added by adamsilverstein 6 months ago.
40894.1.diff (4.3 KB) - added by netweb 6 months ago.
40894.2.diff (4.9 KB) - added by adamsilverstein 6 months ago.
40894.3.diff (6.0 KB) - added by adamsilverstein 3 months ago.
40894.4.diff (5.9 KB) - added by netweb 3 months ago.
40894.5.diff (5.8 KB) - added by adamsilverstein 3 months ago.
40894.6.diff (530.0 KB) - added by adamsilverstein 3 months ago.
40894.7.diff (527.3 KB) - added by adamsilverstein 3 months ago.
40894.8.diff (528.9 KB) - added by adamsilverstein 2 months ago.
40894.9.diff (527.0 KB) - added by adamsilverstein 2 months ago.
remove unrelated changes in gruntfile after merge conflict resolution
40894.10.diff (527.0 KB) - added by adamsilverstein 2 months ago.

Change History (58)

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


6 months ago

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


6 months ago

#3 @adamsilverstein
6 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

In 40894.diff:

Change from browserify to webpack to build media.

  • adds a new webpack.config.js
  • type grunt webpack to run from grunt. Also runs as part of watch and in js:precommit

#4 @adamsilverstein
6 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

@kadamwhite can you take a look at my webpack setup and see if there is anything wrong or that we could do better - I want to keep it very simple at first, then we can iterate and improve - right now i am only trying to match wat browserify is doing for us.

thanks!

#5 @adamsilverstein
6 months ago

  • Keywords needs-testing added

Appreciate testing here from anyone who can build and verify the built files look ok. i did some initial testing and everything worked as expected.

#6 @aduth
6 months ago

The Webpack 3.0 major release was published recently and we might ought to consider targeting it instead

https://medium.com/webpack/webpack-3-official-release-15fd2dd8f07b

The grunt-browserify dependency should be removed from package.json

#7 @adamsilverstein
6 months ago

Thanks for the feedback and for pointing out webpack 3.0 @aduth, I will work on it!

#8 @netweb
6 months ago

Is there a reason the webpack dependencies are in dependencies and not devDependencies in package.json?

@netweb
6 months ago

#9 follow-up: @netweb
6 months ago

@adamsilverstein I've update the patch here with a couple of minor tweaks 40894.1.diff :

  • Adds webpack.config.js to the grunt watch:config task
  • Adds webpack.config.js to the grunt precommit task
  • Moves the webpack dependencies from dependencies to devDependencies

I've not yet tested the output differences from switching to webpack, will test that in the coming days.

#10 in reply to: ↑ 9 @adamsilverstein
6 months ago

Replying to netweb:

@adamsilverstein I've update the patch here with a couple of minor tweaks 40894.1.diff :

thanks, i'll make my updates to your patch.

#11 @adamsilverstein
6 months ago

40894.2.diff

  • upgrade webpack to 3.0.0, add ModuleConcatenationPlugin to config
  • remove grunt-browserify dependency
  • slight code standards cleanup

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


6 months ago

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


5 months ago

#14 follow-up: @kadamwhite
5 months ago

I'm reaching out to some Webpack contributors for additional review

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


5 months ago

#16 in reply to: ↑ 14 @adamsilverstein
5 months ago

Replying to kadamwhite:

I'm reaching out to some Webpack contributors for additional review

@kadamwhite - Any update here? I'd love to get this committed soon so we can get the new built files into trunk for testing.

#17 follow-up: @kadamwhite
3 months ago

@adamsilverstein I ran this by Z Goddard for webpack review, and the only comment I got was that ModuleConcatenationPlugin is meant to only be used in a production setting -- all of the optimization plugins do a certain amount of analysis, so having them run in dev or during a --watch slows things down. If we split the config into a dev and prod config, we could also consider adding some of the other optimizations built in to Webpack down the road.

#18 in reply to: ↑ 17 @adamsilverstein
3 months ago

Replying to kadamwhite:

If we split the config into a dev and prod config, we could also consider adding some of the other optimizations built in to Webpack down the road.

Thanks for the review - great suggestion, I'll work on that!

#19 @adamsilverstein
3 months ago

  • Keywords needs-testing removed

In 40894.3.diff:

  • split config into two files - dev and production
  • use dev config for watch
  • remove webpack.optimize.ModuleConcatenationPlugin use from dev build

Tested out the built files in several browsers, ran tests etc. with good results. @kadamwhite - does this look good to you? Thinking commit this first, then a second commit updating the built files.

#20 @adamsilverstein
3 months ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Future Release to 4.9

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


3 months ago

@netweb
3 months ago

#22 @netweb
3 months ago

Patch 40894.4.diff is a refresh 40894.3.diff to apply cleanly against trunk (no changes) 👍🏼

#23 @aduth
3 months ago

A higher level question:

  • Maybe not in the scope of what's intended here (simply replacing Browserify), but eventually what role would Webpack seek to serve? For example, the Grunt task for uglification is one which Webpack has built-in support (webpack.optimize.OptimizeJsPlugin). The devil's advocate question is: if it's just for basic bundling, are we gaining much over Browserify (+Babelify)?

A couple thoughts on the configuration:

  • At some point if we start to observe significant overlap between development and production configurations, we might want a more scalable approach to sharing common properties (like a common configuration). Conversely, if this is not something we want to aim for early on, we may not even need the file split right now. Gutenberg, for example, has a single webpack.config.js which switches on the process.env.NODE_ENV to add/modify configuration values. This could help with avoiding redundancy on assigning mediaConfig.
  • Should watch: true be assigned into the configuration from Gruntfile.js, or does that make more sense within the configuration itself?
  • We specify stats dependant on the process.env.NODE_ENV but it doesn't appear we set this NODE_ENV anywhere, so it seems unnecessary. And even if it did work as expected, would we want nothing to be output for production builds?

A few minor styling quibbles (in Webpack config files):

  • Should we bother with space aligning variable and object property assignment? Especially with comma-delimited variable declarations, it makes the incorrect assumption that a tab occupies four spaces (in Trac patch, it's obvious with the misalignment since it previews tabs at 8 spaces). Also that we have two spaces following the longest variable name in the respective Webpack configurations.
  • Should be spaces between array square brackets (mediaBuilds) and in require calls, and not after function keyword
  • module.exports object should be concluded with a semi-colon.

#24 @aduth
3 months ago

I guess the obvious response to my own question about scope of Webpack is that if Gutenberg is to be merged with its proposed build tooling (Webpack-based), taking steps in core to accommodate this would ease such a transition. A more interesting question to eventually consider then might be: What does this configuration look like when it scales to accommodate building both media and Gutenberg?

#25 @adamsilverstein
3 months ago

@aduth

Thanks for the review, feedback and questions.

I don't think we need to add minification here, we already handle that in our main build process which we run to package releases. For development, the unminified version is preferable and this approach fits nicely with our existing build tool chain.

One additional use I imagine for webpack is importing/building our npm packages in the wp namespace for inclusion in core.

Regarding splitting the configuration files, I had originally tried a single file and split it up based on this comment and thinking it may help optimize the production/dev builds down the road.

I will try to clean up the configs, removing stats and maybe moving watch:true as well as fixing your other noted format items.

What does this configuration look like when it scales to accommodate building both media and Gutenberg?

Thats a great question we need to figure out the answer to - hopefully we can port over much of what exists in the project. Having webpack in our core toolset will be a prerequisite.

#26 @westonruter
3 months ago

A follow-up ticket to build on the work being done here: #41870 (Code Editor: Add grunt task for building new CodeMirror bundles from external dependencies)

#27 @adamsilverstein
3 months ago

In 40894.5.diff:

Address feedback from @aduth -

  • whitespace fixes
  • remove unused options->stats
  • add missing trailing semicolons

#28 @adamsilverstein
3 months ago

40894.6.diff includes the rebuilt media files. The generated media-views file is slightly larger, probably due to webpack's inline comments. Once run thru minification, the built file is actually about 5% smaller than the file browserify built:

https://cl.ly/3T3r2V3M2X3i/js_2017-09-27_13-39-44.jpg

I did basic testing of media with these webpack built files in ie down to version 9.

Last edited 3 months ago by adamsilverstein (previous) (diff)

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


3 months ago

#30 @schlessera
3 months ago

@adamsilverstein The latest patch seems to remove the Hello Dolly plugin. I suppose that was not intended?

#31 @adamsilverstein
3 months ago

@schlessera Thanks for testing... No, that was not intentional, let me re-upload that patch.

#32 @adamsilverstein
3 months ago

@schlessera can you please try testing again with 40894.7.diff?

#33 @netweb
2 months ago

Webpack is pretty new to me and I've not yet grok'd all the things.... That said.... Please bare with me why I explain my testing timeline....

When I tested 40894.3.diff and refreshed this to become 40894.4.diff I tested this by converting the /build directory to a Git folder ( git init && git add . && git commit), I then renamed the folder to/build-git and ran grunt release from the root of the repo to recreate the /build folder, I then copied the contents of /build to /build-git to compare the differences via git status, in my testing of this there were no changes so I refreshed the patch and uploaded 40894.4.diff and added no additional comment apart for a "thumbs up" to indicate the patch as it stood worked for me as I saw no noticeable changes before and after the patch in the output /build folder.

Moving along, 40894.5.diff at a cursory glance is quite similar to 40894.4.diff and 40894.3.diff ...

And then to 40894.6.diff, this contains significant changes to the src/wp-includes/js/media-audiovideo.js, src/wp-includes/js/media-grid.js, src/wp-includes/js/media-models.js, and src/wp-includes/js/media-views.js files, I originally expected these changes in the 40894.3.diff patch, this is why I performed the testing of the 40894.3.diff patch the way I did above and I thought the reason for why those changes were not in 40894.3.diff was because I wasn't grok'ing webpack correctly and with no difference between before and after applying the patch I thought the changes were good enough, maybe perfect enough that the output to the /build folder had no changes whatsoever and I moved along with nothing worth noting here in the comments.

So, all in all I hadn't grok'd webpack very well and the changes I expected are in fact now in 40894.6.diff and 40894.7.diff, so for whatever reason this is I'll read and re-read and test some more so I do grok what these changes actually are, for now though as I'm basically on holidays with family for the next 6 days I'm unable to test the 40894.7.diff patch.

If others are happy with the state of 40894.7.diff then commit away, I'll catch up when I get back from holidays but I wanted to document the above for my own reference at least... Maybe next week a light bulb moment will occur and I'll see what I clearly missed previously but don't let me hold this back :)

#34 follow-up: @adamsilverstein
2 months ago

@netweb Ha! Thanks for the detailed testing log :) 40894.7.diff is what you should be testing now - it includes the change to the build script as well as the changes to the built media files resulting from the build changes. In the earlier patches, I had left off the built files to make the patch easier to read.

I'm inclined to commit this to get it out to a wider testing audience in beta. If we discover a compatibility issue, we can always revert.

#35 in reply to: ↑ 34 @netweb
2 months ago

Replying to adamsilverstein:

In the earlier patches, I had left off the built files to make the patch easier to read.

Well that's what threw me, these should always be included IMHO regardless of how well things read or not.

I'm inclined to commit this to get it out to a wider testing audience in beta. If we discover a compatibility issue, we can always revert.

As noted I'm travelling for the next week, it would be nice to get a couple of sets of eyes on this before commit IMHO

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


2 months ago

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


2 months ago

#38 @adamsilverstein
2 months ago

40894.8.diff refresh against trunk.

@adamsilverstein
2 months ago

remove unrelated changes in gruntfile after merge conflict resolution

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


2 months ago

#40 @kadamwhite
2 months ago

Notes from slack review with @adamsilverstein :

  • webpack.config.dev.js and webpack.config.prod.js are more standard than webpack-dev.config.js, I'd err on the side of convention here (as established in projects like create-react-app and comments in the webpack docs) for the added specificity
  • We should be adding the most up-to-date versions of webpack and webpack-dev-server, they've both evolved a point or patch release or two since this patch was created

Other than that, looks good, let's do it!

#41 @westonruter
2 months ago

  • Type changed from enhancement to task (blessed)

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


2 months ago

#43 @adamsilverstein
2 months ago

In 40894.10.diff:

  • Rename config files to webpack.config.dev.js and webpack.config.prod.js.
  • Update to the latest version of webpack and webpack-dev-server.

#44 @adamsilverstein
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41752:

Build: Webpack instead of Browserify as JavaScript bundler.

Use Webpack as our bundler of choice, enabling improved JavaScript build chain capabilities.

Props aduth, netweb, kadamwhite, schlessera.
Fixes #40894.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 months ago

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


2 months ago

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


8 weeks ago

Note: See TracTickets for help on using tickets.