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

Build: Reference Lodash as an external module #5933

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
5 participants
@aduth
Member

aduth commented Apr 2, 2018

This pull request seeks to reference Lodash as an external module (window global), rather than include in the build of each bundle.

Why?

  • Bundle size.
    • As implemented, we effectively include a copy of a good chunk of Lodash in each bundle, upwards of a dozen or more copies in fact. There was some hope that the introduction of Webpack 4 and its handling of sideEffects would enable us to at least properly "tree shake" to include only relevant Lodash modules. Preliminary testing, however, shows that lodash-es + sideEffects actually increases end bundle size. Through some debugging, it appears that with Lodash's internal dependency structure, all of Lodash is included anyways in most cases, with additional overhead of linking modules (even concatenated). I think this warrants further exploration, though this has been time-consuming as-is, so may need to be deprioritized in favor of other pursuits.
  • Build times.
    • Not measured, but since externals are effectively pointers to global objects, Webpack shouldn't need to perform module resolution and bundling internal to Lodash itself.

Why not?

  • Conflicts between Lodash and Underscore.js
    • This is not so much an issue with Lodash's _.noConflict, though requires referencing Lodash on the global by its full name lodash (not the _ shorthand).
  • Conflicts between core's enqueue of Lodash vs. plugin enqueuing of Lodash.
    • This is perhaps more concerning, more-so with the no-conflict mode. Lodash is a very popular library, and it's quite likely plugins and themes are already enqueuing it. This becomes akin to themes enqueueing their own versions of jQuery, minus the forethought of dequeueing core's own. In any case, it's likely that one or the other of the two consumers, core or the plugin, would be working with a version they're not anticipating. Lodash has not seen catastrophic breaking changes for some time, so conflicts are not likely, though will be difficult to debug when they do occur.
    • We could enqueue with a handle more likely to be unique to core?

Results There is a total savings of 272.47kb of the total production (minified) bundle size, approximately 15.3% across all of Gutenberg's JavaScript bundles. Lodash's minified bundle is 71.4kb, making for a net savings of 201kb (11.2%). I believe this can be improved further with proper aliasing of lodash-es (see notes below).

Bundle Before After
components/build/index.js 349 KiB 305 KiB
hooks/build/index.js 10.1 KiB 10.1 KiB
plugins/build/index.js 40.4 KiB 20.9 KiB
core-data/build/index.js 54.5 KiB 34.6 KiB
viewport/build/index.js 34.3 KiB 12.2 KiB
data/build/index.js 74.7 KiB 50.4 KiB
utils/build/index.js 49.7 KiB 28 KiB
i18n/build/index.js 18.4 KiB 18.4 KiB
element/build/index.js 25 KiB 8.53 KiB
editor/build/index.js 280 KiB 233 KiB
date/build/index.js 186 KiB 186 KiB
edit-post/build/index.js 114 KiB 81.5 KiB
blocks/build/index.js 547 KiB 522 KiB

Before: 1783.1 KiB
After: 1510.63 KiB
Lodash: 71.4 KiB

Net savings: 201 KiB (11.2%)

Implementation notes:

Babel Preset: We need to drop babel-plugin-lodash for this to work. It's not currently possible to extend our Babel preset configuration to omit this (even programmatically; I'm guessing it partially applies some behaviors upon being loaded?). I have inlined the Babel configuration into the Webpack configuration. This is obviously a temporary solution, and we should either drop this from the packaged Babel plugin, or otherwise allow to be configurable.

lodash-es: Redux includes a couple references to lodash-es/isPlainObject. It would be ideal if these were consolidated to the external we have defined, as lodash.isPlainObject. I expected the proposed configuration would work and, in fact, the built bundle (before minification) includes inline comments about Redux' usage being for an "external" module, though it still includes the module inline. I have not found a related issue, though I wonder if this may be a bug within Webpack. In any case, this does not add much weight, but is duplicated nonetheless, and there is opportunity for further savings.

Testing instructions:

Verify there is no regressions in the build (including plugin-packed), and that the application loads and operates as expected.

@ntwb

This comment has been minimized.

Member

ntwb commented Apr 3, 2018

• Conflicts between core's enqueue of Lodash vs. plugin enqueuing of Lodash.

• This is perhaps more concerning, more-so with the no-conflict mode. Lodash is a very popular library, and it's quite likely plugins and themes are already enqueuing it. This becomes akin to themes enqueueing their own versions of jQuery, minus the forethought of dequeueing core's own.

I'm all for merging this to avoid a Lodash incarnation of the above issue in the future and as Core doesn't currently enqueue Lodash as it is only installed as part of npm devDependencies it is not included in the built release.

Doing this now should be an easy win versus the latter alternative.

@pento

This comment has been minimized.

Member

pento commented Apr 3, 2018

Hah, I started writing exactly this a few minutes ago, before I thought to check if someone else had already done it.

It looks like there are a few plugins that enqueue their own copy of lodash. We can probably reach out to those few plugin authors individually.

@Luehrsen

This comment has been minimized.

Contributor

Luehrsen commented Apr 4, 2018

I am also in favor, as our current approach is to bundle lodash in our custom blocks as well.

To have lodash as an external library would reduce bundle size in the custom block world as well.

@aduth

This comment has been minimized.

Member

aduth commented Apr 5, 2018

Managed to get tests to pass again by faking a .babeljs.js for the babel-jest integration to use. This will be more properly supported in Babel 7. Ideally also we don't have to keep this configuration in Gutenberg, but rather backport to @wordpress/babel-preset-default.

@aduth aduth requested a review from gziolo Apr 5, 2018

@aduth

This comment has been minimized.

Member

aduth commented Apr 9, 2018

Rebased again to include explicit references to Babel dependencies otherwise available only via implicit transitive dependency.

I also noticed in some unrelated effort toward a Babel 7 upgrade that we'll need other changes to the Babel configuration, so it may be a good idea to group these upstream changes.

I was asked in last week's Core JS meeting to justify the inclusion of Lodash, when core already includes Underscore, a similar library. Rather than reiterate common points, I'll simply defer to a well-abbreviated comparison discussion, with a summary of superiority in: features, documentation, testedness, maintenance, performance, consistency, adherence to the language specification (naming like Array#every vs. _.all), and ecosystem ubiquity. While one may argue that Underscore's lack of updates in three years is a benefit in form of stability, it is not as though the project is without bugs and inconsistencies; to me, it rings of abandonware more than anything, though there has been some recent (yet unreleased) commit activity. I'd never made a strong push for Lodash over Underscore, though I do find its conveniences and universality more appealing, when the main benefit of Underscore is only in preserving legacy. I do think performance is an important consideration, particularly for a utility library as these functions will become hot code paths as the application scales. Lodash is almost always many-times faster than its alternatives and, depending on browser/version/function, faster than native equivalents (an interesting comparison video).

I'll acknowledge that one of the original points for its inclusion was its modularity and being able to selectively import features (#289), which is very much ironic given the presence of this pull request to use the entire library instead. I'd not want to dismiss this possibility altogether, as its an area of continuous improvement (see Webpack 4's sideEffects), and Lodash is certainly architected in a way to support future improvements in this area. Of course, the options here appear more limited once we register a common script handle for plugins to depend on. Another part of its original inclusion was that Gutenberg could take advantage of its benefits without exposing the library to plugin authors, avoiding a discussion like the one that's taking place now merely by making it a non-concern. As has been shown with the comparisons in the original comment, we can't really afford to continue paying the bundle size penalty. At worst, I'd still recommend extracting the library, though obscuring it one way or another from general script handle availability.

I'll let the pull request sit until a recap discussion in tomorrow's JS meeting, but I'd like to have this merged ahead of the next release unless there's objection.

cc @adamsilverstein

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Apr 9, 2018

@aduth Thanks for the explanation.

When I asked you to "justify the inclusion of Lodash" I wasn't really asking about underscore vs. lodash - although I appreciate your reasoning there. I was more asking whether its merits are sufficient to warrant not using the already bundled in WordPress underscore. Isn't underscore already being loaded on the post edit page for wp.media?

It sounds like you think they are... Great! If we are switching to Lodash, do you think that means we should switch that across core to avoid loading duplicate, redundant libraries?

@aduth

This comment has been minimized.

Member

aduth commented Apr 11, 2018

Discussed in this week's core JS chat: https://wordpress.slack.com/archives/C5UNMSU4R/p1523367666000160

Consensus is to move forward with this merge, and work toward migrating existing core usage of Underscore.

Related Trac ticket: https://core.trac.wordpress.org/ticket/43733

@aduth aduth merged commit 11b7e94 into master Apr 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the try/lodash-external branch Apr 12, 2018

@aduth aduth added this to the 2.7 milestone Apr 13, 2018

@aduth aduth changed the title from Try: Build: Reference Lodash as an external module to Build: Reference Lodash as an external module Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment