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

Feature: Add webpack plugin to externalize and extract script dependencies #14869

Merged

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 8, 2019

Description

Add a webpack plugin that externalizes WordPress script dependencies and adds a dependency manifest per entrypoint. The goal is to eliminate error-prone process of manually creating and maintaining script dependencies.

A JSON file is output alongside each entrypoint. That looks like (webpack output):

             ./build/annotations/index.js    9.17 KiB      39  [emitted]         annotations
                    ./build/a11y/index.js    1.78 KiB      40  [emitted]         a11y
             ./build/a11y/index.deps.json    16 bytes      40  [emitted]         a11y
      ./build/annotations/index.deps.json    56 bytes      39  [emitted]         annotations

More detail in the README.

Closes Part of #14837

To do

  • Add tests
  • Update README. Document options with examples.
  • CHANGELOG - this package and scripts
  • Fix CI test failures
  • Document in handbook: # #

How has this been tested?

The plugin is put to use in the Gutenberg webpack config. It can be tested using npm run build.

Plugin output results

a11y

["wp-dom-ready"]

annotations

["lodash","wp-data","wp-hooks","wp-i18n","wp-rich-text"]

api-fetch

["wp-i18n","wp-url"]

autop

[]

blob

[]

block-editor

["lodash","react","wp-a11y","wp-blob","wp-blocks","wp-components","wp-compose","wp-core-data","wp-data","wp-deprecated","wp-dom","wp-element","wp-hooks","wp-html-entities","wp-i18n","wp-is-shallow-equal","wp-keycodes","wp-rich-text","wp-token-list","wp-url","wp-viewport","wp-wordcount"]

block-library

["lodash","moment","wp-a11y","wp-api-fetch","wp-autop","wp-blob","wp-block-editor","wp-blocks","wp-components","wp-compose","wp-core-data","wp-data","wp-date","wp-deprecated","wp-editor","wp-element","wp-i18n","wp-is-shallow-equal","wp-keycodes","wp-rich-text","wp-url","wp-viewport"]

block-serialization-default-parser

[]

block-serialization-spec-parser

[]

blocks

["lodash","wp-autop","wp-blob","wp-block-serialization-default-parser","wp-compose","wp-data","wp-dom","wp-element","wp-hooks","wp-html-entities","wp-i18n","wp-is-shallow-equal","wp-shortcode"]

components

["lodash","moment","react","react-dom","wp-a11y","wp-api-fetch","wp-compose","wp-dom","wp-element","wp-hooks","wp-i18n","wp-is-shallow-equal","wp-keycodes","wp-rich-text","wp-url"]

compose

["lodash","wp-element","wp-is-shallow-equal"]

core-data

["lodash","wp-api-fetch","wp-data","wp-deprecated","wp-url"]

data

["lodash","wp-compose","wp-deprecated","wp-element","wp-is-shallow-equal","wp-priority-queue","wp-redux-routine"]

date

["moment"]

deprecated

["wp-hooks"]

dom

["lodash"]

dom-ready

[]

edit-post

["lodash","wp-a11y","wp-api-fetch","wp-block-editor","wp-block-library","wp-blocks","wp-components","wp-compose","wp-core-data","wp-data","wp-editor","wp-element","wp-hooks","wp-i18n","wp-keycodes","wp-notices","wp-nux","wp-plugins","wp-url","wp-viewport"]

edit-widgets

["wp-element"]

editor

["lodash","react","wp-api-fetch","wp-autop","wp-blob","wp-block-editor","wp-blocks","wp-components","wp-compose","wp-core-data","wp-data","wp-date","wp-deprecated","wp-element","wp-hooks","wp-html-entities","wp-i18n","wp-keycodes","wp-notices","wp-nux","wp-rich-text","wp-url","wp-viewport","wp-wordcount"]

element

["lodash","react","react-dom","wp-escape-html"]

escape-html

[]

format-library

["lodash","wp-block-editor","wp-components","wp-element","wp-i18n","wp-keycodes","wp-rich-text","wp-url"]

hooks

[]

html-entities

[]

i18n

[]

is-shallow-equal

[]

keycodes

["lodash","wp-i18n"]

list-reusable-blocks

["lodash","wp-api-fetch","wp-components","wp-compose","wp-element","wp-i18n"]

notices

["lodash","wp-a11y","wp-data"]

nux

["lodash","wp-components","wp-compose","wp-data","wp-element","wp-i18n"]

plugins

["lodash","wp-compose","wp-element","wp-hooks"]

priority-queue

[]

redux-routine

["lodash"]

rich-text

["lodash","wp-compose","wp-data","wp-element","wp-escape-html","wp-hooks"]

shortcode

["lodash"]

token-list

["lodash"]

url

[]

viewport

["lodash","wp-compose","wp-data"]

wordcount

["lodash"]

Types of changes

New feature:
Adds a new package @wordpress/dependency-extraction-webpack-plugin.

New feature:
Uses the plugin in @wordpress/scripts (will require version bump, not included initially). This can be part of another PR but is useful for testing.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@sirreal sirreal changed the title Add/14837 script deps generate on build Feature: Add webpack plugin to externalize and extract script dependencies Apr 8, 2019
@gziolo gziolo added [Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Task Issues or PRs that have been broken down into an individual action to take labels Apr 8, 2019
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

Nice!

I wonder, instead of hardcoding the externals and the handle ref to use, could we make that configurable via an options array? This would allow WordPress plugins to use this for their own externals as well.

So the options config would be an array with two callbacks. The first callback would be used for mapping externals, the second callback would be for mapping to the json output. It could have the shape:

const options = [
    ( request ) => { /* eternalized dependencies callback for mapping external to */ },
    ( request ) => { /* dependencies map callback */ }
];

This way it's somewhat extensible by WP plugins that might want to use this package for non WP externals.

This is just an idea, the actual shape of the configuration option for callbacks could likely be iterated on a bit (eg - should callbacks always be provided and the application merged? Should the wp externals be hardcoded/permanent or can they be overridden?)

@sirreal
Copy link
Member Author

sirreal commented Apr 8, 2019

I wonder, instead of hardcoding the externals and the handle ref to use, could we make that configurable via an options array?

Extensibility and configuration is missing at this time. I had in mind a few options that might be helpful.

Configuration for plugin extensibility allowing plugins provide script dependencies and build bundles that externalize them.

  • requestToExternal: A function that receives module requests (@wordpress/element) and returns a string to externalize the module (wp.element):
    const options = {
      requestToExternal: request => request === '@wordpress/element' ? 'wp.element' : undefined,
    }
  • requestToDependency: A function that receives module requests (@wordpress/element) and returns the WordPress script dependency to include in the manifest list (wp-element):
    const options = {
      requestToDependency: request => request === '@wordpress/element' ? 'wp-element' : undefined,
    }

I can imagine cases where options like these might be helpful as well:

  • useDefaults - include the built-in externals like jquery, lodash, and @wordpres/*. This option would allow this behavior to be enabled/disabled (true/false).
  • injectPolyfill - An option to include wp-polyfill as a dependency for all scripts.

@aduth
Copy link
Member

aduth commented Apr 9, 2019

Related issue: #14837

Am I correct in thinking this should close the issue once merged?

@sirreal
Copy link
Member Author

sirreal commented Apr 9, 2019

Related issue: #14837

Am I correct in thinking this should close the issue once merged?

Yes, thanks. I've updated the description to reflect that.

@gziolo
Copy link
Member

gziolo commented Apr 9, 2019

Am I correct in thinking this should close the issue once merged?

I don't see the following file removed in this PR:
https://github.com/WordPress/gutenberg/blob/master/lib/packages-dependencies.php

If that can be easily squeezed in then sure, we can consider it as closing the issue :)

@aduth
Copy link
Member

aduth commented Apr 9, 2019

Am I correct in thinking this should close the issue once merged?

I don't see the following file removed in this PR:
https://github.com/WordPress/gutenberg/blob/master/lib/packages-dependencies.php

If that can be easily squeezed in then sure, we can consider it as closing the issue :)

I had wondered that as well, whether this would allow for this file to be removed.

@sirreal
Copy link
Member Author

sirreal commented Apr 9, 2019

I've rebased, addressed some feedback, and implemented the configuration outlined in #14869 (comment)

I haven't had an opportunity to test the new functionality yet.

@aduth
Copy link
Member

aduth commented Apr 9, 2019

For those following along, this pull request was discussed during today's JavaScript Chat (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1554818548169400

@sirreal sirreal force-pushed the add/14837-script-deps-generate-on-build branch from d7d964a to 263e409 Compare April 16, 2019 21:13
@sirreal
Copy link
Member Author

sirreal commented Apr 16, 2019

Rebased.

This PR tests well, I didn't notice more significant changes before the old and new output of production build files. There is one subtle difference in the build file but it seems to be legit:

This review prompted me to reconsider the current approach and take some time to review webpack internals.

I've pushed a small change to use this-based externals. Here are a few examples of current output:

/***/ "@wordpress/dom-ready":
/*!*******************************************!*\
  !*** external {"this":["wp","domReady"]} ***!
  \*******************************************/
/*! no static exports found */
/***/ (function(module, exports) {

(function() { module.exports = this["wp"]["domReady"]; }());

/***/ })

/***/ "lodash":
/*!**********************************!*\
  !*** external {"this":"lodash"} ***!
  \**********************************/
/*! no static exports found */
/***/ (function(module, exports) {

(function() { module.exports = this["lodash"]; }());

/***/ })

Notably, object paths may be provided as arrays of strings, for example @wordpress/i18n is now [ 'wp', 'i18n' ]. which results in more robust output this["wp"]["i18n"] compared to wp.domReady. Optimization may still result in the latter, but producing wp.${ packageName } directly could result in invalid output.

With these finishing touches, I'm satisfied with this initial implementation 🙂

@sirreal
Copy link
Member Author

sirreal commented Apr 17, 2019

There is a failing e2e test:

FAIL specs/undo.test.js (8.347s)
● undo › Should undo to expected level intervals

The same test appears to be failing on most recent branches so I suspect the failure is unrelated.

@gziolo
Copy link
Member

gziolo commented Apr 17, 2019

The same test appears to be failing on most recent branches so I suspect the failure is unrelated.

Yes, nothing to worry about. I restarted the job which had the failure. There are a few tests which fail from time to time and we couldn't discover the reason for months now 🤷‍♂️

@gziolo
Copy link
Member

gziolo commented Apr 17, 2019

This is how it looks after changes applied:

development

/***/ "@wordpress/dom-ready":
/*!*******************************************!*\
  !*** external {"this":["wp","domReady"]} ***!
  \*******************************************/
/*! no static exports found */
/***/ (function(module, exports) {

(function() { module.exports = this["wp"]["domReady"]; }());

/***/ })

production

194:function(e,t){!function(){e.exports=this.wp.domReady}()},

It looks like it's identical to what we have in master. Awesome. Let's get this in! 💃

@gziolo gziolo merged commit ae4bce6 into WordPress:master Apr 17, 2019
@gziolo gziolo added this to the 5.6 (Gutenberg) milestone Apr 17, 2019
@gziolo
Copy link
Member

gziolo commented Apr 17, 2019

We made it happen, awesome work @sirreal. It will open new ways for optimizing how JS dependencies are managed for plugin authors and WordPress core as well.

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

@sirreal sirreal deleted the add/14837-script-deps-generate-on-build branch April 17, 2019 10:13
@nerrad
Copy link
Contributor

nerrad commented Apr 17, 2019

@gziolo any idea on when this will be published?

@gziolo
Copy link
Member

gziolo commented Apr 17, 2019

@gziolo any idea on when this will be published?

Whenever WordPress trunk is open for patches after WordPress 5.2 release process is close to ready? We might wait until plugin release. I hope this will happen at the end of the month the latest.

@youknowriad
Copy link
Contributor

Great work @sirreal 👍

@nerrad I expect this to be published once Core trunk branch is open for business. At that point, we could create a wp/5.2 branch here and backport the last Gutenberg release into wp/trunk. I think by the time the next Gutenberg version is released (in two weeks), trunk will probably be open for features...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants