Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Adding exports to CJS modules with built-in names broken by resolve 1.11.1 #394

Closed
bterlson opened this issue Jun 25, 2019 · 5 comments
Closed

Comments

@bterlson
Copy link
Contributor

Resolve changed how it resolves built-in module specifiers like buffer (this also impacted rollup-plugin-node-resolve). The impact is you can no longer add exports for e.g. assert like the following:

  cjs({
    namedExports: {
      assert: ["deepEqual"]
    }
  });

This plugin starts by building a map from resolved module path to declared named exports for that module. The resolve change means the keys for built-in modules went from a resolved path to the like-named npm module to simply returning the built-in name back.

I see three potential fixes here:

  1. similar to Support resolve 1.11.1, add built-in test rollup-plugin-node-resolve#223, we do a two-pass resolution, first attempting to resolve with "/" appended, and falling back if we can't find anything.
  2. if adding named exports for node builtins isn't a thing, we append "/" to any specifier that's a built-in module.
  3. update the documentation to be clear that "buffer" and "buffer/" are two different things, and you should be intentional about which you are declaring named exports for. This would also enable a person to provide different named exports for a built-in and an npm package with the same name. I'm not sure if this is valuable.

Do the maintainers have any thoughts on the best fix here?

@bterlson bterlson changed the title Adding exports to CJS modules with built-in names broken by resolve 1.1 Adding exports to CJS modules with built-in names broken by resolve 1.11 Jun 25, 2019
@bterlson bterlson changed the title Adding exports to CJS modules with built-in names broken by resolve 1.11 Adding exports to CJS modules with built-in names broken by resolve 1.11.1 Jun 25, 2019
mikeharder added a commit to Azure/azure-sdk-for-js that referenced this issue Jun 25, 2019
- Repo has been updated to "rollup-plugin-node-resolve@5.0.2", which is compatible with "resolve@1.11.1"
- Added trailing slash to named exports of modules with built-in names
  - Required when using  "rollup-plugin-commonjs@10.0.0" with "resolve@1.11.1"
  - rollup/rollup-plugin-commonjs#394
- Fixes #3589
@lukastaegert
Copy link
Member

Only some vague thoughts for now:

  • Adding named exports for actual builtins cannot be a thing as builtins MUST be external (and thus automatically have any imports you can imagine); you only need to add imports to stuff that is actually transformed by rollup-plugin-commonjs
  • However it is a thing for modules that have the same name as builtins, i.e. polyfills

Therefore if I understand you correctly, 2. might actually be a viable option

@ljharb
Copy link

ljharb commented Jun 26, 2019

You can use resolve/lib/core, and if the key is in that object and the value is true, it's a builtin module.

@bterlson
Copy link
Contributor Author

That rationale seems sound to me, let's go with 2! Should be nearly a one-liner. And yeah, I'll use Resolve's isBuiltin thing here, rather than following what rollup-plugin-node-builtins does (that plugin should probably just use resolves thing too).

@bterlson bterlson reopened this Jun 26, 2019
@lukastaegert
Copy link
Member

lukastaegert commented Jun 26, 2019

One open question could be how to handle version ranges for builtins that are specified for resolve but I guess we should just use all of them as we do not know on which target system we will run.

@ljharb
Copy link

ljharb commented Jun 26, 2019

@lukastaegert the "core" logic in resolve is written to assume that you're running it on a node version that matches the author's expectations. iow, if they're running on node 6, then only the core modules in node 6 are considered eligible (since the input code isn't written for the target environment, it's written for node)

mikeharder added a commit to mikeharder/azure-sdk-for-js that referenced this issue Aug 1, 2019
- Was required due to rollup/rollup-plugin-commonjs#394
- Fixed in rollup-plugin-commonjs@1.10.1
mikeharder added a commit to Azure/azure-sdk-for-js that referenced this issue Aug 1, 2019
- Was required due to rollup/rollup-plugin-commonjs#394
- Fixed in rollup-plugin-commonjs@1.10.1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants