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

module: Conditional exports #29978

Closed
wants to merge 13 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 15, 2019

This PR implements a way to define conditional "exports" in the package.json, allowing for exports mapping to different values depending on certain conditions that are constants of the resolver in the environment. The proposal is described in more detail in the included documentation at at https://github.com/nodejs/node/compare/master...guybedford:exports-conditions?diff=unified&expand=1&short_path=8e67f40#user-content-conditional-exports, and follows the initial proposal from https://github.com/jkrems/proposal-pkg-exports#2-conditional-mapping.

Currently there are two major driving use cases for this - supporting defining "exports" targets that can allow a custom browser mapping without leaving it to userland to work out how to update the "browser" field or a new custom field to handle this (and dealing with compatibility issues such as for example that the browser field behaves differently to exports semantics in mapping internal specifiers as well as external), and also in supporting packages which can provide a different module file when loading via import or require(), which was one of the points raised as a remaining request for the modules implementation by @ljharb at the last meeting.

The "require" condition can be thought of as a way to "opt-in" to the instancing hazard which we've worked to carefully ensure is never enabled by default. The documentation also carefully mentions this instancing hazard enabled by this condition and strongly suggests ways to avoid it, including proposing a new wrapper module approach to achieve named exports support for CommonJS that is enabled by this feature and avoids the hazard. Both the "require" condition and "node" condition are flagged behind a --experimental-conditional-exports flag.

In addition to the above, other possible use cases of this feature include runtime-specific conditions, to support e.g. "electron" and "react-native" specific entry points in exports, or even conditions such as "production"-specific mapping, without any of these needing to be natively supported by Node.js itself.

This builds on work that has previously been explored through various userland proposals from discussion with RollupJS and Parcel core teams.

The basic approach is to allow "exports" target values to be objects whose keys are condition names, and values are the target to apply for that condition, where conditions apply in a priority order. See the conditional exports docs for the example of browser mapping, and the dual publishing section for the example of defining named exports for an export with a wrapper module technique through this proposal.

Feedback very welcome, this still needs to be discussed at the modules meeting before any consideration of merging, but the more eyes on it the better.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 15, 2019
@mikeal
Copy link
Contributor

mikeal commented Oct 15, 2019

This seems like a reasonable solution to a problem I think we should actually move away from having at all. Let me explain.

ESM is relatively new in Browsers and in Node.js. To date, I know of close to zero modules that will load “natively” (without a compiler) in Node.js’ ESM or in the Browser. All of the modules currently written to ESM syntax are in fact not native ESM and require a compiler in order to be loaded in the Browser and virtually all of those modules are written for the browser and not Node.js.

In other words, to date, JS developers have almost exclusively used ESM syntax to write modules for compilers and not directly for browsers.

But the goal of ESM is not to provide a new syntax for compilers. The goal, particularly in the Browser, has been to enable truly dynamic loading of native JS modules for the Browser. As Node.js develops native support for ESM this presents an opportunity.

If done correctly, ESM in Node.js could lead us towards “universal JS modules” that work in Browsers and in Node.js without any compiler (or even a local install step). To their credit, the people who have been working on ESM have been moving in this direction (removing globals not accessible to the browser, for instance).

The solution in this PR, designed for use by compilers, pushes in the other direction. Solving any problem with package.json means you have not solved it in a way that will work for native ESM in the Browser. Yes, this is limiting. Yes, this is actually kind of painful right now because it’s difficult to write modules that work everywhere. But, if we fix these issues with features that move away from truly universal modules we will make this transition even worse, and could end up in this position indefinitely without a native solution.

I think a reasonable question to ask of any feature related to ESM in Node.js is “how does this work without a compiler or local install when loading the same code in the Browser?” That’s a question that Node.js has never had to ask before, but it’s a necessary one if we want to reach a universal module format.

@guybedford
Copy link
Contributor Author

guybedford commented Oct 15, 2019

@mikeal "exports" has been carefully designed to be fully compatible with import maps generation in the browser. This type of conditional exports feature remains identically compatible with that process.

It's a valid point though whether we should be branching on entry points (as in this PR), or relying entirely on code splitting and conditional code-based branching.

But this point makes the opposite argument to what you are making - that if we want to avoid compilers, we should be using import map generation tools that can handle conditional branching of exports resolution, in order to avoid needing to compile and optimize module code branches for different types of environment conditions.

That is, this feature enables import map generators to support universal mapping generation without compilation, as the environment optimizations can already be done by package authors at publish time.

@rektide
Copy link

rektide commented Oct 15, 2019

I am really not sure what the use case is here. I don't understand the base premise of why a package could or should have multiple sets of exports. That seems like a very confusing idea to introduce to packages & I'd like to understand more clearly both what for & why it is necessary?

@mikeal
Copy link
Contributor

mikeal commented Oct 15, 2019

@mikeal "exports" has been carefully designed to be fully compatible with import maps generation in the browser. This type of conditional exports feature remains identically compatible with that process.

This is the problem, you just used the word “generation.” Who does the generation? How does my static universal module generate anything? This assumes an install or compile step on a system that is then deployed in some way. That workflow is actually relatively new in web development and native ESM gives us an opportunity to support an older and simpler workflow in which people consume JS code from the internet using a URL and publish code statically to URLs, none of which requires a local system in which things are installed and then deployed.

Whether a compiler/generator runs at install, publish, or load time, it’s still required in the workflow if such a step is necessary.

I appreciate that this feature is in line with what can be supported in import maps, but I’m actually skeptical that this sort of up-front generation of an import map is going to be sufficient. This is probably a much longer side thread that I don’t want to have derail this one, but if you consider what you can do with a backend or service worker for dynamic module loading the case for generated static mapping is unclear given how much less you can do with it. I think import maps are a nice feature to have, particularly in the case that someone is writing them by hand rather than generating them, but I’ve not been convinced that they solve the range of concerns people seem to think they solve.

@guybedford
Copy link
Contributor Author

I am really not sure what the use case is here. I don't understand the base premise of why a package could or should have multiple sets of exports. That seems like a very confusing idea to introduce to packages & I'd like to understand more clearly both what for & why it is necessary?

If you look at the number of different "main" fields people use in package.json files, that pretty much explains the use case. The "browser" field was created in the package.json to allowing build tools to provide a different entry point when in the browser. These approaches can also be used by CDNs when serving modules to the browser. This can be useful to avoid code paths that do not apply for the environment the package is being used in, exactly to cater to universal package use cases without needing optimizations to avoid including unnecessary code in all environments (there is no need to ship Node.js code to the browser or vice versa). This branching can otherwise be done via dynamic import and top-level await though, although at the cost of non-statically-analyzable latency.

With Node.js supporting "exports" users may want to be able to expand on its semantics in other environments, as happened with the "main" proliferations into other forms. And we do risk userland coming up with new sorts of fields to tackle that. This demonstrates a way to capture those other cases under the same field helping tools to share syntax and semantics. It's certainly a wide topic, and I would suggest using the modules repo issue at nodejs/modules#401 or creating a new issue there to branch off this discussion further if not directly related to the PR.

@mikeal
Copy link
Contributor

mikeal commented Oct 15, 2019

IMO, the fact that we only have one “module” field in package.json for ESM modules is a feature, not a bug. It means we’re defining a single entry point for universal ESM modules. I may be in the minority on this one though.

@ljharb
Copy link
Member

ljharb commented Oct 15, 2019

@rektide every package with multiple files already has multiple exports.

@mikeal similarly, only providing a single entry point for ESM, while CJS has infinite, would be a problem.

Also, as has been said, the "browser" field has for years provided a mapping of N files for bundlers, in its object form.

As for "without a compiler" - browsers will never have all the capabilities node has, and so there will never be a guarantee that any node module will work in a browser without transformation. While it's certainly possible to write a node module that can work in a browser without changes, not just for ESM but also for CJS (it's just harder), imo it's not ever going to be an achievable goal in the abstract.

@mikeal
Copy link
Contributor

mikeal commented Oct 15, 2019

only providing a single entry point for ESM, while CJS has infinite, would be a problem.

My position is the exact opposite, that infinite entry points is exactly what keeps us from universal modules ;)

doc/api/esm.md Outdated Show resolved Hide resolved
@ljharb

This comment has been minimized.

@mikeal
Copy link
Contributor

mikeal commented Oct 15, 2019

@ljharb you’re making it sound as though adding features don’t have consequences. They certainly do.

The ecosystem we have today is a direct result of how the module system, in tandem with npm, was engineered. The fact that two dependencies can load different versions of a sub-dependency is a feature, one that isn’t available in most languages, and which had the effect of creating an explosively large ecosystem and creating a much more complicated de-duplication task for front end bundlers.

I don’t think it’s productive to say “you can still do this other thing even though we have features that incentivize you not to solve problems that way.” If universal modules are a goal, which I believe they are, the features in Node.js need to point towards that and not away from it.

@GeoffreyBooth
Copy link
Member

@mikeal and @ljharb and @rektide we have a separate issue at nodejs/modules#401 for general discussion of this proposal. Do you mind taking your conversation there? And we can keep the thread on this PR specific to discussion of the code in the PR.

@jkrems

This comment has been minimized.

@jkrems
Copy link
Contributor

jkrems commented Oct 15, 2019

Moved my comment to nodejs/modules#401 (comment).

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

I've updated the "require" condition to instead be the inverse "module" condition. I think its meaning would be clear to most as it matches the package.json "module" main meaning.

The benefit of this inversion for CommonJS / ES module branching is that it allows us to support a flag to enable or disable this feature, something like node --no-module-condition or node --module-condition depending on which way we want the default to be.

The nice thing is such a flag would not break application code by design, whereas a flagged "require" condition would always be a breaking change to opt-out of (CommonJS can no longer load the package as CommonJS).

Considering a flag that does not cause breaking behaviour either way gives us more options for the middle ground in our disagreements on this hazard.

@guybedford
Copy link
Contributor Author

I've pushed a commit to this PR to allow the following shorthand exports configuration:

{
  "exports": {
    "module": "./index-module.mjs",
    "default": "./index-other.cjs"
  }
}

which is effectively sugar for the dot main in exports, based on the simple rule that the first key string does not start with a dot.

While it could be seen as a little magical, I think it's important to have a very simple syntax for the base main case, which is really the main consideration for us in unflagging here.

Feedback further welcome.

@jkrems
Copy link
Contributor

jkrems commented Oct 23, 2019

Do we need to invert the flag to support --no-module-condition? Wouldn't it also work to say --prefer-require-condition that makes ESM look at require conditionals? That feels cleaner.

@devsnek
Copy link
Member

devsnek commented Oct 23, 2019

(reposting from #30051 (comment))

i think it would be better for the ecosystem to just not support this for the time being. the current way this is exposed to people writing modules is incredibly complex and confusing, and having code like this littered around is definitely not an improvement to codebases. yes, we lose the ability to ship packages that are both supported on old node and have named exports on new node, but we gain pure simplicity.

@GeoffreyBooth
Copy link
Member

yes, we lose the ability to ship packages that are both supported on old node and have named exports on new node, but we gain pure simplicity.

I think users would prefer the capabilities to the simplicity. Any users who want simplicity can just not use conditional exports.

@devsnek
Copy link
Member

devsnek commented Oct 23, 2019

@GeoffreyBooth it creates complexity for those consuming the module too, and they didn't opt into it. it creates complexity for tools which now have to parse our weird package.json configs, and they didn't opt into it. it creates complexity for package managers, which may need to parse that information in order to better optimize packages on disk, and they didn't opt into it.

@GeoffreyBooth
Copy link
Member

Package authors will publish dual packages no matter what we tell them, whether using conditional exports or 'pkg/module' and anything downstream of such packages (other dependents, app developers, build tools) opt into that complexity by choosing to depend on such a package.

Most of the issues related to the hazard via conditional exports are still present even in a 'pkg/module' approach. Even if we don’t ship conditional exports, the recommended approach for 'pkg/module' is still the “ESM wrapper” approach in #30051.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

Two suggestions that are docs-only.

doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

guybedford and others added 3 commits November 7, 2019 15:19
Co-Authored-By: Jan Olaf Krems <jan.krems@gmail.com>
@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 7, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor Author

Here's the CI error that keeps coming up just on that distribution -

/libnode/gen/src/node/inspector/protocol/Protocol.o.d.raw   -c

17:20:35 In file included from /usr/include/c++/6/functional:55:0,
17:20:35                  from /usr/include/c++/6/memory:79,
17:20:35                  from ../deps/v8/include/v8.h:21,
17:20:35                  from ../src/util.h:27,
17:20:35                  from ../src/inspector/node_string.h:6,
17:20:35                  from /home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj/gen/src/node/inspector/protocol/Forward.h:10,
17:20:35                  from /home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj/gen/src/node/inspector/protocol/Protocol.h:10,
17:20:35                  from /home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj/gen/src/node/inspector/protocol/Protocol.cpp:7:
17:20:35 /usr/include/c++/6/tuple: In constructor 'constexpr std::_Head_base<_Idx, _Head, true>::_Head_base(_UHead&&) [with _UHead = std::default_delete<node::inspector::protocol::InternalResponse>; unsigned int _Idx = 1u; _Head = std::default_delete<node::inspector::protocol::Serializable>]':
17:20:35 /usr/include/c++/6/tuple:69:35: internal compiler error: Segmentation fault
17:20:35   : _Head(std::forward<_UHead>(__h)) { }

there is nothing here associated with this PR, and this is the only env failure, so I'm going to go ahead with landing.

guybedford added a commit that referenced this pull request Nov 8, 2019
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
guybedford added a commit that referenced this pull request Nov 8, 2019
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in 2367474.

@guybedford guybedford closed this Nov 8, 2019
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #29978
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@desmap
Copy link

desmap commented Jul 29, 2020

@guybedford It's still not clear from the docs how to use this in the runtime: Let's say we have

"exports": {
  ".": {
    "next": "./dist/browser-shim.js",
    "default":"./index.js"
  }
}

How can I set next later in my runtime? I tried node --env=next but node 14.6 tells me this is a bad option.

Fwiw, I can't use the built-in browser because Next.js uses also a node server where I want to have the browser shim to be used too, hence I need my own condition and not browser)

@jkrems
Copy link
Contributor

jkrems commented Jul 29, 2020

@desmap IIRC right now setting custom conditions may require using loader hooks (e.g. adding next to conditions in the resolve hook).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.