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

Build: Add new Webpack plugin to handle library default export #6935

Merged
merged 3 commits into from
May 25, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 24, 2018

Description

Closes #6748.

Previously: #6584 (comment)

The intention with consuming from the WordPress npm organization packages is that each module would have itself exposed as an equivalent camel-case form as a property on window.wp. This already works well for most packages, allowing developers to use e.g. wp.i18n.__ (the __ named export from @wordpress/i18n).

However, for packages which have a default export, the usage is not as expected:

wp.domReady.default( function() {

Note the .default, which ideally should be omitted, calling instead as wp.domReady( fn ).

I played with the different approaches proposed in #6748 and ended up implementing a custom Webpack plugin, which works very similar to what libraryExport: 'default' does as explained in Webpack docs:

libraryExport: "default" - The default export of your entry point will be assigned to the library target:

// if your entry has a default export of `MyDefaultModule`
var MyDefaultModule = _entry_return_.default;

Plugin's implementation is based on the ExportPropertyMainTemplatePlugin core plugin. The only difference in here is that instead of applying this transformation to all entry points, we will be able to provide the list of chunks where this should be applied:

new LibraryExportDefaultPlugin( [ 'dom-ready' ].map( camelCaseDash ) )

How has this been tested?

Make sure npm run dev and npm run build work properly.

It should be possible to execute the following code on the JS console when running Gutenberg:

wp.domReady( () => {} );

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo requested review from aduth and youknowriad May 24, 2018 10:40
@gziolo gziolo self-assigned this May 24, 2018
@gziolo gziolo added [Type] Build Tooling Issues or PRs related to build tooling npm Packages Related to npm packages [Type] Task Issues or PRs that have been broken down into an individual action to take labels May 24, 2018
@gziolo gziolo added this to the 3.0 milestone May 24, 2018
@gziolo gziolo requested a review from ntwb May 24, 2018 10:50
@gziolo gziolo force-pushed the add/library-export-default-plugin branch 2 times, most recently from cfe7696 to d46845a Compare May 24, 2018 13:08
@gziolo gziolo force-pushed the add/library-export-default-plugin branch from d46845a to f71b609 Compare May 24, 2018 13:16
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks (and works) great 👍

Unfortunate that we can't detect this automatically. Guessing that's impossible / prohibitively difficult to implement. At least for the time being, I'm more concerned with avoiding developers coming to expect the .default property to exist.

@aduth aduth merged commit de04b61 into master May 25, 2018
@gziolo gziolo deleted the add/library-export-default-plugin branch May 25, 2018 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [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.

2 participants