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

Support pkg.exports.node field when building for Node.js #695

Closed
ctavan opened this issue Dec 7, 2020 · 8 comments
Closed

Support pkg.exports.node field when building for Node.js #695

ctavan opened this issue Dec 7, 2020 · 8 comments

Comments

@ctavan
Copy link
Contributor

ctavan commented Dec 7, 2020

  • Rollup Plugin Name: @rollup/plugin-node-resolve
  • Rollup Plugin Version: 11.0.0
  • Rollup Version: 2.34.2
  • Operating System (or Browser): any
  • Node Version: 15.x
  • Link to reproduction: https://repl.it/@ctavan/rollup-plugin-repro

Expected Behavior

@rollup/plugin-node-resolve@11.0.0 added support for pkg.exports entrypoints through #540.

However it seems like the node keyword is currently not respected which causes trouble when bundling for Node.js environments.

The specific pkg.exports definition that doesn't work as expected is https://github.com/uuidjs/uuid/blob/334ef62c330d92f8ca376d09087e8ee9abc5cc12/package.json#L21-L31:

{
  "exports": {
    ".": {
      "node": {
        "module": "./dist/esm-node/index.js",
        "require": "./dist/index.js",
        "import": "./wrapper.mjs"
      },
      "default": "./dist/esm-browser/index.js"
    },
    "./package.json": "./package.json"
  }
}

Since apparently the node keyword is ignored, rollup picks up the default key, which in this case is a browser build.

Node.js supports the node keyword and this is supported in webpack@5 as well

It has been reported as a bug to uuidjs/uuid#544

Actual Behavior

When browser: false, respect the node keyword, just like Node.js and webpack do.

Additional Information

/cc @guybedford @LarsDenBakker @TrySound @lukastaegert since you were involved in earlier discussions around pkg.exports and uuid as a widely used canary, see rollup/rollup#3514 & webpack/webpack#11014 😉

Edit: Here's the reference where @guybedford proposed the pattern used in uuid's pkg.exports field: uuidjs/uuid#468 (comment)

@shellscape
Copy link
Collaborator

Have you seen #693?

@lukastaegert
Copy link
Member

In the examples mentioned, I do not see the node condition specified explicitly. If you read the plugin documentation, you will see that the only conditions set by default are ['default', 'module', 'import']. I am not sure whether the absence of browser should automatically imply node, especially since browser is to my knowledge not yet integrated into package.exports resolution anyway (you would still need to specify the browser condition explicitly). That being said, there is definitely room for improvement, at the very least on documentation level but also to make things clearer for the user with less screws to turn.

@ctavan
Copy link
Contributor Author

ctavan commented Dec 8, 2020

@shellscape thanks for the pointer. I must have missed that issue when searching before raising this report. After reading through it I'm still unsure if it really solves this issue out of the box.

@lukastaegert I was referring to

a) The node.js documentation at https://nodejs.org/api/packages.html#packages_conditional_exports which states:

Node.js supports the following conditions out of the box:

  • "import" - matches when the package is loaded via import or import(), or via any top-level import or resolve operation by the ECMAScript module loader. Applies regardless of the module format of the target file. Always mutually exclusive with "require".
  • "require" - matches when the package is loaded via require(). The referenced file should be loadable with require() although the condition matches regardless of the module format of the target file. Expected formats include CommonJS, JSON, and native addons but not ES modules as require() doesn't support them. Always mutually exclusive with "import".
  • "node" - matches for any Node.js environment. Can be a CommonJS or ES module file. This condition should always come after "import" or "require".
  • "default" - the generic fallback that always matches. Can be a CommonJS or ES module file. This condition should always come last.

b) The Webpack documentation at https://webpack.js.org/guides/package-exports/#target-environment which states:

The following conditions are set depending on the target environment:

Condition Description Supported by
browser Code will run in a browser. webpack
electron Code will run in electron. webpack
worker Code will run in a (Web)Worker. webpack
worklet Code will run in a Worklet.  
node Code will run in Node.js. webpack, Node.js
deno Code will run in Deno.  
react-native Code will run in react-native.

Now when looking at https://rollupjs.org/guide/en/#quick-start the example "For Node.js"

rollup main.js --file bundle.js --format cjs

somewhat suggests that the resulting bundle is supposed to run in Node.js. That's why I made the assumption that when bundling for CJS I would automatically get the Node.js-style package.exports resolution. But maybe I'm misinterpreting things here a bit.

I do agree that browser: false can probably not automatically mean node: true

So maybe now that package.exports support arrived in the node-resolve plugin it's now also time for a target: option for the node-resolve plugin, similar to what webpack does? Otherwise I see a lot of potential for confusion around this topic.

@lukastaegert
Copy link
Member

Definitely, and no matter how we go from here, there should be clear examples in the documentation that show-case the main use-cases (i.e. bundling for Node and bundling for the web).

@shellscape
Copy link
Collaborator

@ctavan can you double check this against the latest version?

@ctavan
Copy link
Contributor Author

ctavan commented Apr 2, 2021

@shellscape I've updated the Replit to use @rollup/plugin-node-resolve@11.2.1 but the issue is still there.

If I try to execute the resulting bundle with Node.js I get:

$ node output/bundle.js 
\/home/runner/rollup-plugin-repro/output/bundle.js:12
    throw new Error('crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported');
    ^

Rollup still seems to pick up the browser build instead of the node build.

@guybedford
Copy link
Contributor

This can now be configured when using the exportsConditions: ['node'] option of plugin-node-resolve. If there are any further issues here please do let us know though.

@ctavan
Copy link
Contributor Author

ctavan commented May 15, 2021

I have configured exportsConditions: ['node'] instead of browser: false in https://replit.com/@ctavan/rollup-plugin-repro#rollup.config.js but rollup still seems to pick up the browser bundle from uuid.

What am I doing wrong?

FTR: I realized it must read exportConditions: ['node'] instead of exportsConditions: ['node'] (there was an "s" too much). It now works! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants