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

node-resolve plugin no longer supports "default" require import #402

Closed
alexlafroscia opened this issue May 20, 2020 · 7 comments
Closed

Comments

@alexlafroscia
Copy link

alexlafroscia commented May 20, 2020

  • Rollup Plugin Name: node-resolve
  • Rollup Plugin Version: v8.0.0
  • Rollup Version: 1.25.2 (through broccoli-rollup)
  • Operating System (or Browser): macOS
  • Node Version: 12.16.2

How Do We Reproduce?

https://repl.it/@alexlafroscia1/UnsightlyAliceblueExternalcommand

Expected Behavior

The plugin function would be accessible by requiring @rollup/plugin-node-resolve

Actual Behavior

The package no longer has a "default export" of the plugin function when importing through Node rather than ESM. While configuring Rollup directly supports the ESM style, configuring Rollup i
indirectly (like in our case, through the Broccoli wrapper plugin) does not.

Since the default ESM export is a function, I would expect the same to be true when requiring it in Node.

@shellscape
Copy link
Collaborator

Thanks for opening an issue. Citing the issue template:

Issues without minimal reproductions will be closed! Please provide one by:

  1. Using the REPL.it plugin reproduction template at https://repl.it/@rollup/rollup-plugin-repro
  2. Provide a minimal repository link (Read https://git.io/fNzHA for instructions).
    These may take more time to triage than the other options.
  3. Using the REPL at https://rollupjs.org/repl/

Please add a reproduction and we'll be happy to triage further.

@alexlafroscia
Copy link
Author

Reproduction through repl.it

https://repl.it/@alexlafroscia1/UnsightlyAliceblueExternalcommand

@shellscape shellscape reopened this May 20, 2020
@shellscape
Copy link
Collaborator

Looks like this is expected. Changelog contains notes which address this: https://github.com/rollup/plugins/blob/master/packages/node-resolve/CHANGELOG.md#v800

@alexlafroscia
Copy link
Author

I guess so. As a user, it seems pretty weird that ESM supports a default export but Node does not. Especially when all of the documentation shows usage of the default export.

@alexlafroscia
Copy link
Author

I'll close this, as the change seems intentional. The changelog entries just make this pretty hard to notice

Especially when ESM still does support a default. You have to do a fair amount of digging to find out that just the node usage changed.

@shellscape
Copy link
Collaborator

You have to do a fair amount of digging to find out that just the node usage changed.

You're of course welcome to that observation, but I would disagree that it takes a fair amount. It takes a normal/expected amount for major versions imho.

@omsmith
Copy link

omsmith commented May 20, 2020

Just to add a note, given the changelog I was a bit confused at first when looking at this today since I am using ESM via a rollup.config.mjs. However, since node doesn't use the module field in package.json, it's also impacted by the CommonJS-targetting breaking change.

- import nodeResolve from '@rollup/plugin-node-resolve';
+ import nodeResolvePlugin from @rollup/plugin-node-resolve';
+ const { nodeResolve } = nodeResolvePlugin;

alexanderby added a commit to darkreader/darkreader that referenced this issue May 23, 2020
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

3 participants