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 "[email protected]", which is compatible with "[email protected]"
- Added trailing slash to named exports of modules with built-in names
  - Required when using  "[email protected]" with "[email protected]"
  - 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
mikeharder added a commit to Azure/azure-sdk-for-js that referenced this issue Aug 1, 2019
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