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

Module and import strategy #941

Closed
nylen opened this issue May 30, 2017 · 7 comments
Closed

Module and import strategy #941

nylen opened this issue May 30, 2017 · 7 comments

Comments

@nylen
Copy link
Member

nylen commented May 30, 2017

We need to give some thought to the way we offer our code to be consumed by various pieces of software:

  1. within Gutenberg itself
  2. within other parts of WordPress, especially given that we will eventually be integrating Webpack into more places
  3. in plugins, which may or may not use any kind of JS build tooling.

Previously, we were duplicating a lot of code in our built files - in particular, things in components are duplicated across blocks/build/index.js and editor/build/index.js. Some details at #929 (comment).

Current status

#929 addressed this particular concern by de-duplicating our Webpack build and making all of our exported code available via global variables like wp.components.Button for example. We can still use import in our ES6 code here (and in other projects that use Webpack). This is the approach I would recommend, at least for now, because it allows us to support all of the 3 goals above pretty transparently.

In order to avoid this duplication going forward, this implies that any import statement in a WordPress dependencies block needs to have the following form:

/**
 * WordPress dependencies
 */
import { Button, withFocusReturn } from 'components';

(note, components corresponds directly to wp.components, and there is no path associated with the components directory).

Concerns

@youknowriad raised the concern that it would be nice to import higher-order components differently:

import { withFocusReturn } from 'components/higher-order';

After #929 this would cause withFocusReturn to be included in the blocks build (if this import was performed from within the blocks code), and it would be duplicated across multiple builds if imported this way in multiple builds.

Alternatives and Next Steps

I'm not aware of a clean Webpack configuration that addresses this issue, and I'm not sure if we would want to do that anyway (requiring imports from only the top level of other builds seems nice and clean to me).

An alternative would be to drop the import statement, recognizing that we are actually playing inside a large ecosystem that needs to make its code available for various uses other than ES6 modules compiled within the same Webpack build:

/**
 * WordPress dependencies
 */
const { Button } = wp.components;
const { withFocusReturn } = wp.components.higherOrder;

Whatever we choose here, we should probably also create some lint rules to ensure consistency.

@youknowriad
Copy link
Contributor

I have a concern with making the wp.components available to the community, I think we should make it available to Core but making it available to the community means we should ensure backwards compatibility for all the components there.

I do think there's value in making it available (same UI for core and plugins) but we may want to defer this a bit unless it's something stable enough.

@nylen
Copy link
Member Author

nylen commented May 30, 2017

Yes, we should definitely be thinking about public API contracts and backwards compatibility here. In particular this has bitten the media library in the past. Example: https://core.trac.wordpress.org/changeset/31935/

I have a concern with making the wp.components available to the community, I think we should make it available to Core but making it available to the community means we should ensure backwards compatibility for all the components there.

How can we achieve this? The previous approach was not workable as it caused around 200 kb of minified code to be duplicated, mostly from components.

@youknowriad
Copy link
Contributor

How can we achieve this?

Just explicitly documenting that it's not public API could be enough for me.

@ellatrix
Copy link
Member

I posted a comment on the key codes PR (#940 (comment)), which might be valuable here too. I think we should prefix our exported modules with wp/ because they're also global, just in a different context.

@nylen
Copy link
Member Author

nylen commented May 31, 2017

See also #935 (comment). If possible, I'd like to bring our test build in line with our dev/production builds by making it use multiple entry points as well.

@nylen
Copy link
Member Author

nylen commented Aug 3, 2017

Just explicitly documenting that it's not public API could be enough for me.

Circling back around to this - if we make an API available, it is public, and changing it later will probably break people's sites.

@mtias
Copy link
Member

mtias commented Nov 20, 2017

We have made some changes here to have stricter consistency: #3533

@mtias mtias closed this as completed Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants