#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)
Change History (58)
This ticket was mentioned in Slack in #core by aduth. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.
7 years ago
#4
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years ago
Thanks for the feedback and for pointing out webpack 3.0 @aduth, I will work on it!
#8
@
7 years ago
Is there a reason the webpack dependencies are in dependencies
and not devDependencies
in package.json
?
#9
follow-up:
↓ 10
@
7 years ago
@adamsilverstein I've update the patch here with a couple of minor tweaks 40894.1.diff :
- Adds
webpack.config.js
to thegrunt watch:config
task - Adds
webpack.config.js
to thegrunt precommit
task - Moves the webpack dependencies from
dependencies
todevDependencies
I've not yet tested the output differences from switching to webpack, will test that in the coming days.
#10
in reply to:
↑ 9
@
7 years 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
@
7 years ago
- 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.
7 years ago
This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.
7 years ago
#14
follow-up:
↓ 16
@
7 years 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.
7 years ago
#16
in reply to:
↑ 14
@
7 years 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:
↓ 18
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years 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.
7 years ago
#22
@
7 years ago
Patch 40894.4.diff is a refresh 40894.3.diff to apply cleanly against trunk (no changes) 👍🏼
#23
@
7 years 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 theprocess.env.NODE_ENV
to add/modify configuration values. This could help with avoiding redundancy on assigningmediaConfig
. - Should
watch: true
be assigned into the configuration fromGruntfile.js
, or does that make more sense within the configuration itself? - We specify
stats
dependant on theprocess.env.NODE_ENV
but it doesn't appear we set thisNODE_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
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years ago
In 40894.5.diff:
Address feedback from @aduth -
- whitespace fixes
- remove unused options->stats
- add missing trailing semicolons
#28
@
7 years ago
40894.4.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:
I did basic testing of media with these webpack built files in ie down to version 9.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#30
@
7 years ago
@adamsilverstein The latest patch seems to remove the Hello Dolly plugin. I suppose that was not intended?
#31
@
7 years ago
@schlessera Thanks for testing... No, that was not intentional, let me re-upload that patch.
#32
@
7 years ago
@schlessera can you please try testing again with 40894.7.diff?
#33
@
7 years 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:
↓ 35
@
7 years 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
@
7 years 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.
7 years ago
This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.
7 years ago
#38
@
7 years ago
40894.8.diff refresh against trunk.
This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.
7 years ago
#40
@
7 years ago
Notes from slack review with @adamsilverstein :
webpack.config.dev.js
andwebpack.config.prod.js
are more standard thanwebpack-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!
This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.
7 years ago
#43
@
7 years ago
In 40894.10.diff:
- Rename config files to
webpack.config.dev.js
andwebpack.config.prod.js
. - Update to the latest version of
webpack
andwebpack-dev-server
.
In 40894.diff:
Change from browserify to webpack to build media.
webpack.config.js
grunt webpack
to run from grunt. Also runs as part ofwatch
and injs:precommit