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

feat(pluginutils): add native node es modules support #419

Merged
merged 4 commits into from
Jun 1, 2020

Conversation

TrySound
Copy link
Member

@TrySound TrySound commented May 26, 2020

Rollup Plugin Name: pluginutils

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Enable native node es modules via "exports" field in package.json.
Added custom plugin to generate nested package.json with {"type": "module"}
as an alternative to mjs extension usage. This works similar to rollup distribution.
https://unpkg.com/browse/rollup@2.10.9/dist/es/

This change is necessary before migrating all other plugins.

Ref #412 #413

Enable native node es modules via "exports" field in package.json.
Added custom plugin to generate nested package.json with `{"type": "module"}`
to prevent mjs extension usage.
@@ -4,6 +4,19 @@ import typescript from '@rollup/plugin-typescript';

import pkg from './package.json';

function emitModulePackageFile() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's export this output plugin from https://github.com/rollup/plugins/blob/master/shared/rollup.config.js so we can reuse it

TrySound added a commit to TrySound/estree-walker that referenced this pull request May 26, 2020
A blocker for rollup/plugins#419

emitModulePackageFile is used to avoid .mjs extension.
@TrySound
Copy link
Member Author

Looks like there is a blocker
Rich-Harris/estree-walker#23

@TrySound
Copy link
Member Author

@shellscape Bundling estree-walker would eliminate the problem. What do you think?

@shellscape
Copy link
Collaborator

@TrySound it's got a pretty small bundle footprint of 2.9kb raw. That should be fine.

Comment on lines +78 to +82
"exports": {
"require": "./dist/cjs/index.js",
"import": "./dist/es/index.js"
},
"module": "./dist/es/index.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

question about the choice to nest these within a directory: our prior file format was index.[format].[extension] for output files. e.g. index.es.js. what's the reasoning behind the choice to put them into directories?

Copy link
Member Author

Choose a reason for hiding this comment

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

es/package.json with {type:module}

Copy link
Member Author

Choose a reason for hiding this comment

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

As an alternative to .mjs extension.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Can you explain in the PR description why we need mjs alternative? I've added a bunch more people to review. We're going to need to get majority buy-in to move forward with this since it's a pretty big paradigm shift.

@danielgindi
Copy link
Contributor

@TrySound can you please explain why we need to shift from mjs?

@TrySound
Copy link
Member Author

I just don't like it. And to be consistent with rollup. So no strong opinion.

@shellscape
Copy link
Collaborator

And to be consistent with Rollup

That seems the most compelling reason. I really don't like how it litters dist but if that stops the complaining about Node 14+ imports then I guess that's what we do.

If we're going to adopt this, we should also revisit comments by @Andarist where dist files were named pluginutils.js instead of index.js, as that would also follow parent Rollup's convention.

@shellscape
Copy link
Collaborator

I'll be following up with a PR to address the comment from above:

If we're going to adopt this, we should also revisit comments by @Andarist where dist files were named pluginutils.js instead of index.js, as that would also follow parent Rollup's convention.

@shellscape shellscape merged commit c599182 into master Jun 1, 2020
@shellscape shellscape deleted the pluginutils-mjs branch June 1, 2020 14:03
LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
* feat(pluginutils): add native node es modules support

Ref rollup#412 rollup#413

Enable native node es modules via "exports" field in package.json.
Added custom plugin to generate nested package.json with `{"type": "module"}`
to prevent mjs extension usage.

* Export emitModulePackageFile from shared config

* Bundle estree-walker

* Swap options
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

Successfully merging this pull request may close these issues.

6 participants