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

Library Export Default Webpack Plugin: Rewrite as CommonJS #15710

Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented May 17, 2019

Extracted from: #15226

This pull request seeks to expose the source of LibraryExportDefaultWebpackPlugin directly as its source, omitting it from the Babel transpilation build step. The proposed benefit is to reduce overhead by limiting the number of Babel-transpiled packages, to accurately reflect the current source as being CommonJS and, depending on the outcome of #15226, avoid dependencies on a built package from the project's own build tooling (Webpack). The main downside is fragmentation on which packages are and aren't transpiled by Babel.

Testing instructions:

Verify build succeeds:

npm run build

@youknowriad
Copy link
Contributor

Do we have a rule for dropping the src folder when we use CommonJS, why not keeping the src folder regardless of the way the package is shipped and just change the entry point?

@@ -0,0 +1,5 @@
## Unreleased
Copy link
Contributor

@youknowriad youknowriad May 17, 2019

Choose a reason for hiding this comment

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

Is it master or unreleased?

Copy link
Member

Choose a reason for hiding this comment

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

Master - we should update our docs

Copy link
Member Author

Choose a reason for hiding this comment

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

Master - we should update our docs

See: #15740

@aduth
Copy link
Member Author

aduth commented May 17, 2019

Do we have a rule for dropping the src folder when we use CommonJS, why not keeping the src folder regardless of the way the package is shipped and just change the entry point?

The src folder is a trigger for some build tooling. There was some similar discussion before at #13329 (comment) . There's also this one which... probably works fine, but again operates on the assumption that src = subject to transpilation:

// Finds all packages which are transpiled with Babel to force Jest to use their source code.
const transpiledPackageNames = glob( 'packages/*/src/index.js' )
.map( ( fileName ) => fileName.split( '/' )[ 1 ] );

These could be updated to avoid making assumptions, I think. In the meantime, however, it seems safer to keep them out of that directory.

There are some some benefits to this as well:

  • index.js is the default main, so it's both conventional (well-understood) and avoids a custom override in package.json
  • src as a concept seems to imply there's a build / dist, which is not true in this instance

@youknowriad
Copy link
Contributor

Makes sense. Thanks 👍

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Yes, let’s do it 👍

@gziolo
Copy link
Member

gziolo commented May 20, 2019

See: https://medium.com/@nodejs/announcing-a-new-experimental-modules-1be8d2d6c2ff

Add “type”: “module” to the package.json for your project, and Node.js will treat all .js files in your project as ES modules.

There is also commonjs value proposed.

We should add this new field to all packages and start using it with the script which picks which packages should be transpiled. This is going to solve the issue with special handling for src folder.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I now see that here the lock file needs to get regenerated as well. Our script on Travis seems to not work properly anymore.

@aduth
Copy link
Member Author

aduth commented May 20, 2019

Our script on Travis seems to not work properly anymore.

In another pull request (I can't specifically recall), we tracked it back to an issue where npm ci is intentionally not meant to update package-lock.json (source), but that it should exit with an error code if the package-lock.json is incorrect. In the previous occurrence, npm ci was completing successfully despite npm install resulting in changes, so I'd deemed it an issue of npm ci. I think it also affects only specific modifications to the file.

@aduth aduth force-pushed the update/webpack-plugins-common-js-library-export-default branch from 368a8a1 to 3108745 Compare May 20, 2019 13:42
@aduth
Copy link
Member Author

aduth commented May 20, 2019

Resolved package-lock.json and "Unreleased" -> "Master" changes in rebased 3108745.

@aduth aduth merged commit 83a0815 into master May 20, 2019
@youknowriad youknowriad deleted the update/webpack-plugins-common-js-library-export-default branch May 20, 2019 14:12
@youknowriad youknowriad added this to the 5.8 (Gutenberg) milestone May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants