Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build: Remove Custom Bundler #16379

Closed
wants to merge 7 commits into from
Closed

Build: Remove Custom Bundler #16379

wants to merge 7 commits into from

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Jul 19, 2017 •
edited
Loading

This PR Seeks to remove our custom bundler from the build process so that we can use webpack via cli.

why

  • simplify. using a custom bundling script opens the door for straying from the straight and narrow which can make upgrade paths more difficult.
  • scope-hoisting/tree shaking: its mostly unrelated, but while implementing the pr to enable these features, it was difficult to enable es6 within the bundler. By removing the bundler entirely, we can rely on just renaming webpack.config.js --> webpack.config.babel.js
  • speed. the uglification changes take advantage of both parallelization + a cache. will probably double our docker build speeds again :)

how
the custom bundler only runs during prod builds and also only does two things.

  1. asset filepath generation. We can use assets-webpack-plugin
  2. uglification: We can use UglifyJsPlugin. We should wait for the soon-to-be-merged uglification parallelization: feat: add support for parallelization && caching (options.parallel) webpack-contrib/uglifyjs-webpack-plugin#77
  3. wait for 1.0 release

To Test:
run calypso in these environemnts:

  • local dev initial build
  • local dev inc build
  • docker build

@samouri samouri self-assigned this Jul 19, 2017
@samouri samouri requested review from blowery and ehg July 19, 2017 20:00
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/build/get-rid-of-bundler

@matticbot matticbot added the [Size] L Large sized issue label Jul 19, 2017
@samouri samouri added Build and removed [Size] L Large sized issue labels Jul 19, 2017
@samouri samouri force-pushed the update/build/get-rid-of-bundler branch from 7b2b291 to b48e941 Compare July 19, 2017 20:01
@matticbot matticbot added the [Size] L Large sized issue label Jul 19, 2017
@gziolo
Copy link
Member

gziolo commented Jul 19, 2017

By removing the bundler entirely, we can rely on just renaming webpack.config.js --> webpack.config.babel.js

Love this PR :)

@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Jul 20, 2017
gziolo
gziolo reviewed Jul 21, 2017
View reviewed changes
server/pages/index.jade
script(src=urls[ jsFile + '-min' ])
if chunk
script(src=urls[ chunk + '-min' ])
script(src=urls[ 'manifest' ])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is might invalidate all cached files in the browser. Do we keep the same file names for production assets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you mean. Can you explain the problem a bit more?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure urls of JS files served on production remain the same. Otherwise returning visitors will have to load them again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they'll have to load them again when this change lands regardless. Changing the filename to drop the min should be fine...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed @blowery . I don't think there is an issue here

blowery
blowery reviewed Jul 21, 2017
View reviewed changes
server/pages/index.js Outdated
urls[ name + '-min' ] = asset.url.replace( '.js', '.min.js' );
}
} );
forEach( assets, ( asset, name ) => urls[ name ] = asset.js );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll admit it took me a minute to parse out that asset.js was asset[ 'js' ] and not a straight filename :D

blowery
blowery reviewed Jul 21, 2017
View reviewed changes
webpack.config.js Outdated
@@ -252,9 +258,9 @@ if ( process.env.DASHBOARD ) {
webpackConfig.plugins.unshift( new DashboardPlugin() );
}

if ( process.env.WEBPACK_OUTPUT_JSON ) {
if ( process.env.WEBPACK_OUTPUT_JSON || process.env.NODE_ENV === 'production' ) {
webpackConfig.devtool = 'cheap-module-source-map';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside, but is this still the best source map to use?

Copy link
Contributor Author

@samouri samouri Jul 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that with uglify on we need to turn on uglify's source maps instead: https://github.com/webpack-contrib/uglifyjs-webpack-plugin#options

blowery
blowery reviewed Jul 21, 2017
View reviewed changes
server/bundler/bin/bundler.js
}

// run one minifier per cpu core
async.eachLimit( files, os.cpus().length, minifyFile, done )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay one more async dep gone

blowery
blowery reviewed Jul 21, 2017
View reviewed changes
server/boot/index.js Outdated
@@ -40,9 +39,6 @@ function setup() {
// setup logger
app.use( morgan( 'dev' ) );
} else {
assets = require( 'bundler/assets' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how was this used server side?

Copy link
Contributor Author

@samouri samouri Jul 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to generate script tag urls as far as I can tell 🤔

@samouri
Copy link
Contributor Author

samouri commented Jul 26, 2017 •
edited
Loading

I rudely put out a request for a release date of 1.0: webpack-contrib/uglifyjs-webpack-plugin#95

after that its full steam ahead with this pr!

@samouri samouri mentioned this pull request Sep 5, 2017
Closed
@samouri samouri force-pushed the update/build/get-rid-of-bundler branch from 2b227e7 to c156d68 Compare September 12, 2017 20:08
@samouri
Copy link
Contributor Author

samouri commented Sep 12, 2017 •
edited
Loading

I am currently in favor of pushing through with this PR using the current stable version built into webpack instead of waiting for the new 1.0 with parallel and caching.

We'll be ready to accept the upgrade trivially when it comes out (in a month? impossible to know) and I believe moving forward sooner will make scope hoisting easier to adopt. I'll do a speed comparison so that its easier to weigh the tradeoff.

@samouri
Build: Remove Custom Bundler
fd502d7 
turn on UglifyJSPlugin for production build

enable parallel + file cache

goodbye assets middleware!

readFileSync instead so that first serve doesnt 500
@samouri
get back on older uglify plugin
d30c30d
@samouri
small tweaks
dbb7d71
@samouri samouri force-pushed the update/build/get-rid-of-bundler branch from 0666f47 to dbb7d71 Compare September 13, 2017 19:19
@samouri samouri changed the base branch from master to fix/async-load-minified2 September 13, 2017 19:19
@samouri
Copy link
Contributor Author

samouri commented Sep 14, 2017 •
edited
Loading

build time for this PR on my machine (macbook pro 2016) was 4:20

@samouri samouri mentioned this pull request Sep 15, 2017
Merged
@samouri
Copy link
Contributor Author

samouri commented Oct 6, 2017

Closing because we merged: #18074

@samouri samouri closed this Oct 6, 2017
@samouri samouri deleted the update/build/get-rid-of-bundler branch October 6, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blowery blowery blowery left review comments

@gziolo gziolo gziolo left review comments

@ehg ehg Awaiting requested review from ehg

Assignees

@samouri samouri

Labels
Build Framework [Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samouri @matticbot @gziolo @blowery @designsimply