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 type module commonjs to the corresponding packages #15841

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 27, 2019

Description

There was announced a new --experimental-modules support for Node. It contains also package.json “type” field:

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

If some of your project’s files use CommonJS and you can’t convert your entire project all at once, you can either rename those files to use the .cjs extension or put them in a subfolder containing a package.json with { “type”: “commonjs” }, under which all .js files are treated as CommonJS.

For any file that Node.js tries to load, it will look for a package.json in that file’s folder, then that file’s parent folder and so on upwards until it reaches the root of the volume. This is similar to how Babel searches for .babelrc files. This new approach allows Node.js to use package.json for package-level metadata and configuration, similar to how it is already used by Babel and other tools.

This PR marks all packages which contain commonjs syntax with its corresponding type.

This allows us to use a more predictable way to filter out packages which don't need to be processed with Babel. It will also make it possible to use src subfolder in packages which don't need to transpiled.

How has this been tested?

  • npm run dev
  • npm run build

Future considerations

This probably will have to be revisited once Node enables full support for modules. However, I don't think it's going to happen any time soon.

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Type] Build Tooling Issues or PRs related to build tooling labels May 27, 2019
@gziolo gziolo self-assigned this May 27, 2019
@gziolo gziolo added the Needs Technical Feedback Needs testing from a developer perspective. label May 27, 2019
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Thanks @gziolo for keeping this updated. I have no objection here.

* @return {boolean} Whether file is a directory.
*/
function isModuleType( file ) {
const { type = 'module' } = require( path.resolve( PACKAGES_DIR, file, 'package.json' ) );
Copy link
Member

Choose a reason for hiding this comment

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

We should confirm that 'module' is the default here. I don't think it is.

@gziolo
Copy link
Member Author

gziolo commented May 29, 2019

This PR was discussed during Core JS chat yesterday (link requires registration on WordPress Slack):

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

There is also a related question from @aduth in #15841 (comment).

I did some further research to clarify how this new feature is supposed to work.

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

Currently, it is not possible to create a package that can be used via both require(‘pkg’) and import ‘pkg’. There are efforts underway to address this, and may involve changes to the above. In particular, Node.js might choose a field other than “main” to define a package’s ES module entry point. While we are aware that the community has embraced the “module” field, it is unlikely that Node.js will adopt that field as many of the packages published using “module” include ES module JavaScript that may not evaluate in Node.js (because extensions are left off of filenames, or the code includes require statements, etc.). Please do not publish any ES module packages intended for use by Node.js until this is resolved.

Also in nodejs/node#26745:

You can use package.main to set an entry point for a module

  • the file extensions used in main will be resolved based on the type of the module

All our “main” entries in packages point to code which uses CommonJS! It means that we need to wait until it's clarified what's going to be an entry point for module type.

I can confirm based on http://2ality.com/2019/04/nodejs-esm-impl.html#filename-extensions that commonjs is the default type. This makes this proposal not viable. I figured out that we can reason about the package type based on the “module” field. Only those transpiled with Babel have this field defined. I will open another PR where I will move some of the logic which this PR adds.

@gziolo gziolo closed this May 29, 2019
@gziolo gziolo removed the Needs Technical Feedback Needs testing from a developer perspective. label May 29, 2019
@gziolo gziolo deleted the update/type-module-package branch May 29, 2019 07:51
@gziolo
Copy link
Member Author

gziolo commented May 29, 2019

Opened #15879 as an alternative.

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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants