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.

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

Framework: Remove packages build from build, dev scripts #15226

Merged
merged 8 commits into from May 30, 2019

Conversation

@aduth
Copy link
Member

commented Apr 26, 2019

Previously: #15219
Related: #14560

This pull request seeks to update the Gutenberg-specific Webpack configuration to avoid running Babel as part of the Webpack build. As described at #15219 (comment), the files processed by Webpack are already transpiled by Babel as a step immediately prior. Including babel-loader is therefore redundant, wasted effort.

With this change, I'd measured approximately a 57% decrease in the time it takes to run npx wp-scripts build (~16.7s to 7.1s, measured using time).

Testing Instructions:

  1. Reset your repository using git clean -fdx (Warning: This will forcefully remove any uncommitted files)
  2. Run npm run build
  3. Verify that JavaScript outputs both in build/*/*.js and in packages/*/build/*.js are transpiled as expected by Babel (e.g. no ES2015 or JSX)
@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

This change disables also source maps support:

config.module.rules.unshift( {
test: /\.js$/,
use: require.resolve( 'source-map-loader' ),
enforce: 'pre',
} );

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

This change disables also source maps support:

That's unfortunate. In which case, we might need the "rethink" I'd described at #14560 (comment) sooner than later.

Some initial ideas here include:

  • Name the loaders (perhaps a name property of the rule object) to allow for simpler filtering
    • This may not work, since Webpack validates configuration shapes against a schema and might object to a custom property being added
  • Create a utility which returns a rule configuration, perhaps similarly by a name, e.g. createRule( 'babel' )
  • Consider not inheriting or duplicating configurations from the default

@gziolo gziolo requested a review from nosolosw Apr 30, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@nosolosw any thoughts on this issue?

@nosolosw

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I've been reading a bit more about the relationship between bin/packages/build, wp-scripts and the Gutenberg webpack overrides.

  • I did a little search to see if webpack could output the babel transpilation in addition to create the output bundle so we can get rid of the bin/packages/build script. It looks like it can't - at least I didn't found how.

  • I learned that bin/packages/build.js is used to prepare files for the npm release (build and build-module folders within the own package folder) while scripts build/start prepares the bundle for WordPress (output ./build/packagename/index.js used by client-assets.php). These are different processes to me, so I was pondering why they aren't uncoupled? For example: how about removing npm run build:packages from the build and dev commands? I guess this would mean that the release process needs to be updated to add npm run build:packages before npm run build. Is there any other thing that'd be affected?

@gziolo

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@nosolosw, in fact you need only npm run build:packages to publish to npm. As of today wp-scripts build depends on the previous command as it treats WordPress packages as they would be located in node_modules folder. Actually, they exist there as symbolic links.
It might also turn out that we use 2 different Babel configs for both commands so the output may differ slightly with this patch.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

  • These are different processes to me, so I was pondering why they aren't uncoupled? For example: how about removing npm run build:packages from the build and dev commands? I guess this would mean that the release process needs to be updated to add npm run build:packages before npm run build. Is there any other thing that'd be affected?

That's an interesting proposal. I think it could work. The main downside I see is that when someone runs npm run build, they may expect all possible build variations (i.e. the packages transpilation). I don't know that someone would often find themselves in this position, so it seems like it might be okay to overlook, as long as we're careful to ensure that packages are correctly built when we expect them to be.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Actually, I don't think anything else needs to change:

  • npm run publish:* all use npm run build:packages directly already
  • ./bin/build-plugin-zip.sh doesn't care about or include JavaScript files from packages/

@aduth aduth force-pushed the update/webpack-skip-babel branch from 59097fe to ba0ef74 May 8, 2019

@aduth aduth changed the title Framework: Omit Babel processing from Webpack configuration Framework: Remove packages build from build, dev scripts May 8, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Well, the failing build would at least demonstrate why the packages build would be necessary, or at least specifically for packages upon which the Webpack configuration itself depends:

> wp-scripts build
/home/travis/build/WordPress/gutenberg/node_modules/webpack-cli/bin/cli.js:244
				throw err;
				^
Error: Cannot find module '@wordpress/custom-templated-path-webpack-plugin'

const DependencyExtractionWebpackPlugin = require( '@wordpress/dependency-extraction-webpack-plugin' );

I guess it'd worked for me locally because I'd already had a built copy available.

One option might be to rewrite this Webpack plugin as plain CommonJS.

@aduth aduth requested review from ajitbohra, nerrad, ntwb and youknowriad as code owners May 8, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

There were two packages which needed to be adapted to CommonJS:

  • @wordpress/custom-template-path-webpack-plugin
  • @wordpress/library-export-default-webpack-plugin

Puzzlingly, both of these were already implemented as CommonJS ([1], [2]), but with folder structures and main / module configured as if they were to be transpiled.

This wasn't the only problem, unfortunately. Since Webpack will interpret the package.json of a file, and because the main files of build/index.js will not exist without npm run build:packages, it would error that the package could not be resolved. In 3d851b9 I adapted something similar to the Jest modules mapping to direct Webpack to the src if it's present.

I'm not sure I'm as comfortable with this approach as in the original proposal, since:

  • We totally disregard a package's main if a src directory exists (though the same can be said of our Jest setup)
  • We miss out on potential performance optimizations to npm run build:packages, e.g. as considered in #15230

It is quite fast though: I see npm run build times of ~9.6s (compared to upwards of 50s or longer in master), where wp-scripts build accounts for only 5-7s of the time.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

From the failing build, there are more things to consider:

  • The end-to-end tests reference packages which must be built (source)
  • The JavaScript unit tests task in JavaScript relies on npm run build to have produced the build artifact from @wordpress/block-serialization-spec-parser's custom build script
    • This would be separately resolved by #15229
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I think one of the main reasons to run babel there was to generate the "messages" in prod as these are not generated by the packages build process.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

I think one of the main reasons to run babel there was to generate the "messages" in prod as these are not generated by the packages build process.

Can you explain more what you mean by this? I'm not sure what "messages" you're referring to.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@aduth I was referring to the babel-plugin-make-pot but just remembered that we don't use it anymore right? Which makes it useless for me to run babel at the application level.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@youknowriad Correct, we no longer use @wordpress/babel-plugin-makepot as of #12559.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

There were two packages which needed to be adapted to CommonJS:

  • @wordpress/custom-template-path-webpack-plugin
  • @wordpress/library-export-default-webpack-plugin

Puzzlingly, both of these were already implemented as CommonJS ([1], [2]), but with folder structures and main / module configured as if they were to be transpiled.

These are extraneous tasks which should probably be done separately anyways, which I've proposed:

  • #15709 Custom Template Path Webpack Plugin: Rewrite as CommonJS
  • #15710 Library Export Default Webpack Plugin: Rewrite as CommonJS
@gziolo

This comment has been minimized.

Copy link
Member

commented May 20, 2019

they do because they consume the packages?

Can you teach Parcel to use sources the same way @aduth proposed it for webpack flow?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@gziolo you can using aliases, that said, I'm not particularly attached to the playground using parcel, it just seemed easier to setup that way.

https://parceljs.org/module_resolution.html#alias

@gziolo
Copy link
Member

left a comment

I think I have a preference for this option as it's strictly equivalent to what Core does and I don't think going throught the files once more is too time consuming if we drop babel.

This PR proposes the alternative approach which seems to be much faster. However, the downside is that we no longer will be testing whether the generated distribution version of packages works as expected. I think it's a good trade-off for development to use sources directly. How about we mix two approaches and use the version optimized for speed with npm start and the version optimized for stability with npm run build?

Aside:

  • we use sources of all packages with unit tests
  • we use build folders of all packages with code that maintains e2e tests

We should take some strategic decisions to align the approach after we land this PR. I would be fine in having the approach to be optimized for speed thus using sources everywhere but having a flag which would allow using packages prepared for npm on demand to run with one of the jobs on Travis - maybe it's fine to do it on Travis only?

Aside 2:

$ time npm run build:packages

real    0m29.945s
user    0m36.459s
sys     0m3.108s
@@ -157,13 +157,13 @@
"clean:packages": "rimraf ./packages/*/build ./packages/*/build-module ./packages/*/build-style ./packages/*/node_modules",
"prebuild:packages": "npm run clean:packages && lerna run build",
"build:packages": "node ./bin/packages/build.js",
"build": "npm run build:packages && wp-scripts build",
"build": "wp-scripts build",

This comment has been minimized.

Copy link
@gziolo

gziolo May 20, 2019

Member

At the moment lerna run build is needed only for one package which isn't part of the application so it's fine as is.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

A thought: Rather than expecting a project to import and manipulate the default Webpack configuration object, could we implement the default configuration so that it responds to flags passed to wp-scripts build? For example, a wp-scripts --no-babel could exclude the Babel loader from the configuration, without needing to manipulate the configuration object directly.

There might need to be some consideration on how we name these flags, depending on how much we want to expose internal implementation detail of the build. Considering that Babel is integrated pretty tightly into the build tool (local Babel configurations are detected), this flag seems reasonable enough to me in its current form.

@aduth aduth force-pushed the update/webpack-skip-babel branch from 3d851b9 to 3b7201e May 20, 2019

@aduth aduth requested a review from mkaz as a code owner May 20, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

In the rebase, I revert back to the original proposal to omit Babel from the Webpack build, using a new fla as in my previous comment #15226 (comment) . As discussed, there are some other considerations for how we might want to take this, but I think this is a good middle-ground compromise for a resolution to the immediate problem that we run Babel twice in both npm run build and npm run dev, so long as we're content that a --no-babel flag is something we're comfortable to commit to.

@@ -156,7 +156,7 @@
"clean:packages": "rimraf ./packages/*/build ./packages/*/build-module ./packages/*/build-style ./packages/*/node_modules",
"prebuild:packages": "npm run clean:packages && lerna run build",
"build:packages": "node ./bin/packages/build.js",
"build": "npm run build:packages && wp-scripts build",
"build": "npm run build:packages && wp-scripts build --no-babel",

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 20, 2019

Contributor

I personally don't like the no-babel option. I wonder if we're not doing ourselves a disservice in assuming that using wp-scripts is good as we're proofing it while in reality, we tweak its config in a way that makes it different from what the users of wp-scripts do. I'd personally rather just embrace the fact that Gutenberg is a special case here and use webpack directly.

This comment has been minimized.

Copy link
@gziolo

gziolo May 21, 2019

Member

Using webpack directly won't solve the issue fully. The shared webpack config which by default has this Babel integration built-in is something that needs to be tweaked.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 21, 2019

Contributor

I think it's important to understand why we're using webpack at the root level of Gutenberg to understand the issue and what is the "common" webpack config Gutenberg setup has with wp-scripts setup.

The answer is simple, there's nothing really important to share between these two webpack configs aside the DependencyExtractionWebpackPlugin plugin for me. Is this sufficient to try to include checks in the common config instead of just duplicating the 4/5 lines that are common?

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

@youknowriad What do you envision as the purpose of wp-scripts build, and how do you imagine that developers should configure its usage? Currently, you can configure your own Babel setup or fall back to the default one, but you can't opt out of it. It's for this reason that I see there being value in the flag, regardless of what we're doing with it.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@aduth for me, ideally the users don't touch the babel config at all, it's not even an option. Its main target are plugin/theme developpers that want a tool to build their plugins with sane defaults but don't want to understand/invest in the config files of all the build tools.

That doesn't mean they shouldn't offer any config, but I see these as more "user-facing" options. babel is not something the user of wp-scripts should have to learn/know about.

Some of these options could be:

  • entry points
  • output folder
  • enable/disable RTL stylesheets
  • enable/disable i18n ...

but not something related to the syntax itself

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@youknowriad It's still not very clear to me. You're suggesting that enabling ES2015 transpilation is not something we should be including as a feature of the build script? Or is it more about how we're surfacing specific tools (implementation details) like Babel?

If it is the latter, I have no qualms against alternative naming for an option. Something like --no-esnext perhaps?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@aduth In my mind both :). I think the tool should offer sane defaults and plugin authors shouldn't think about whether they enable ES6 or anything.

That said, I don't want to block that PR because of that. I'd prefer to just rely on webpack and custom config at the root level but admittedly, we already support custom configs in the tool (that goes against what I think the goal of the tool should be), so it's not going to be worst with this new option :)

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

I don't feel compelled to rush this in. Or at least, I want to better understand what our expectations are for wp-scripts build, since we don't seem to all be in agreement. Regardless of whether Gutenberg uses its own Webpack configuration†, it would help to have a clearer picture of what specifically we mean by "sane defaults" and "user-facing options" in its standard usage. This would help me in understanding what would be an ideal solution here and generally.

† I have opinions on this as well, and I tend to agree with earlier points of dogfooding, but I feel this is being a distraction in the conversation.

@aduth aduth force-pushed the update/webpack-skip-babel branch from 3b7201e to eb309dc May 25, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

Okay, I relent. The latest rebase will just avoid extending the default configuration. As previously noted, there's not a whole lot to reimplement to preserve the existing behavior (devtool, source-map-loader, DependencyExtractionWebpackPlugin).

I think this would be reasonable to address the immediate issue of the Babel double-run and, if we choose to, sort out an alternative solution which supports this, the goals of wp-scripts build, and which reinstates the common configuration.

@@ -134,5 +147,7 @@ module.exports = {
},
},
] ),
new DependencyExtractionWebpackPlugin( { injectPolyfill: true } ),

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 27, 2019

Contributor

There are two things that are removed by this change. I believe the webpack bundle analyzer and the live reload config. I'm not sure if people find these useful or not but I thought I'd mention.

This comment has been minimized.

Copy link
@aduth

aduth May 28, 2019

Author Member

There are two things that are removed by this change. I believe the webpack bundle analyzer and the live reload config. I'm not sure if people find these useful or not but I thought I'd mention.

Oof. Well, I can go conservative and leave the behavior unaffected, with the downsides of (a) more duplication and (b) more changes to package.json and package-lock.json or (b) just drop them. I've personally not made use of them (though in the former case, I have done bundle analysis by just calling webpack --stats directly).

Not sure yet how I want to proceed.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 29, 2019

Contributor

I'm personally fine with just removing those as not much used but it could be done separately and I think we can move forward here.

This comment has been minimized.

Copy link
@aduth

aduth May 30, 2019

Author Member

I'm personally fine with just removing those as not much used but it could be done separately and I think we can move forward here.

I'll commit to reintroducing it if anyone mentions it.

@aduth aduth merged commit c12859d into master May 30, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the update/webpack-skip-babel branch May 30, 2019

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Framework: Remove packages build from build, dev scripts (WordPress#1…
…5226)

* Framework: Omit Babel processing from Webpack configuration

* Framework: Remove packages build from build, dev scripts

* Build Tooling: Update Webpack config to operate on source files

* Revert "Build Tooling: Update Webpack config to operate on source files"

This reverts commit 8ca8c18.

* Revert "Framework: Remove packages build from build, dev scripts"

This reverts commit 0021e5c.

* Scripts: Add flag `--no-babel` for excluding Babel processing

* Framework: Exclude Babel processing by build script flag

* Framework: Avoid extending default configuration in Webpack build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.