Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Conditional exports proposal #401

Closed
guybedford opened this issue Oct 15, 2019 · 16 comments
Closed

Conditional exports proposal #401

guybedford opened this issue Oct 15, 2019 · 16 comments
Labels
modules-agenda To be discussed in a meeting

Comments

@guybedford
Copy link
Contributor

Out of discussion from the last meeting I have just posted a core PR with a conditional exports proposal here - nodejs/node#29978.

This enables e.g. browser mappings in exports, as well as the ability for require() and import to get different modules, as mentioned by @ljharb.

Note that the feature of supporting the "require" condition is something that we can agree to enable or not and is somewhat independent of the proposal.

An interesting consequence of the proposal was that it provides a way to do named exports for CJS packages without getting the instance hazard, as users can effectively write their own wrapper modules.

Hopefully having the proposal fleshed out and implemented allows us to properly discuss these concerns further and decide either way both on the require handling, and on whether we want to support a conditional approach like this for browser mapping or whether we want to leave this problem to userland.

@jkrems
Copy link
Contributor

jkrems commented Oct 15, 2019

Reacting to @mikeal's concern in nodejs/node#29978 (comment), emphasis mine:

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.

I think you're spot on: The ideal we should work towards is universal JS. That is why I pushed hard for one variant being just default. Which actually is the... default without conditional mappings. It should be the one-JS implementation, independent of the runtime.

But realistically there are differences in platform APIs. node will never have a global document or location object that real browser code could interact with.[1] Certain APIs around file system, cryptography, network IO, etc. will need time to converge if they ever fully will. I don't think we have the luxury to put modules on hold until that happened.

And this isn't even touching on questions like "should we enable packages to provide dedicated development builds".

To me this proposal represents a shift from how node used to think about runtimes: The default assumption is no longer that files are explicitly targeting node or are using require-specific file formats and APIs. That is now an exceptional condition worth documenting in the package metadata. And the set of packages that require them will hopefully decrease over time.

As for "targeted for compilers": Yes, for the browser all package meta data is either targeting compilers/bundlers/package managers or users that write long config files by hand. If a user is mapping each bare specifier manually, this will tell them which file they should pick in a fairly readable way. I don't think the fact that sometimes they have to pick from multiple options and sometimes they "just" have to dig through each package in their dependency tree to discover the proper string value is a huge difference there.

[1] I'm not counting the deno location object which is more "inspired by location" than actually matching what reasonable browser code might expect.

@ljharb
Copy link
Member

ljharb commented Oct 15, 2019

To clarify my comments from that thread, i think that more universal JS is a great goal, but since fully universal JS between disparate platforms that have uncommon ground (filesystem, network access, DOM, cookies, “having a URL for the application”, etc) it’s never going to be fully achievable.

We shouldn’t make node worse to comply with browser constraints that need not constrain us.

@gr2m
Copy link

gr2m commented Oct 15, 2019

Thank you everyone involved! I think the proposal is well thought through, and it does address my current problem with ES Modules.

As the author of a universal module which abstracts away environment-based API differences, I want users of my module to have a single import statement and not worry about it. I can't see an alternative to conditional exports to make that possible right now, especially for sub-dependencies. I think it's a necessary evil. /cc @mikeal nodejs/node#29978 (comment)

Example

I've created universal-user-agent to make sure I can easily set a meaningful userAgent for http requests as required my most API providers, such as GitHub's API. The package is used in @octokit/request which in turn is used in @octokit/rest.

Without the conditional exports suggested in this proposal, I'm not sure how I can author a low-level module such as universal-user-agent without putting a lot of extra burden to every dependent module. /cc @mikeal nodejs/node#29978 (comment)

@MylesBorins
Copy link
Contributor

I'm coming around to this proposal, but not with default conditional mapping for Node.js core. Specifically I think that we should support the Object syntax but we should not support more than a single entry officially.

{
  "exports": {
    "./": { "node": "./index.mjs" }
  }
}

This maintains the behavior we currently have, guards against the divergent specifier hazard, but allows tooling + ecosystem to extend exports in an ergonomic way.

{
  "exports": {
    "./": { "node": "./index.mjs", "browser": "./index-browser.mjs" }
  }
}

The above would "just work" in node, and tools building import maps / bundling / dynamically building graphs can utilize the extra meta data for their use cases. This behavior is already able to be done with our implementation of export maps in a slightly less clean way with array fallbacks.

{
  "exports": {
    "./": [{ "browser": "./index-browser.mjs" }, "./index.mjs"]
  }
}

So I see this proposal, minus the multiple default supported entry points, as an ergonomic enhancement. We could later explore --env or a similar pattern to allow folks to support multiple entry points for things like prod / dev. This extensibility seems amazing from an ecosystem growth perspective... but will still maintain that one specifier means one thing in a runtime.

I think we should aim to land this proposal ASAP behind a flag so we can start playing with / experimenting. In theory we could add an additional flag for more default entry points e.g. "require" || "default" so people can experiment with everything. I'd very much like us to ensure we are building something the ecosystem can use and will want to extend while minimizing how much of the decisions / magic are baked into Node.js core.

@jkrems
Copy link
Contributor

jkrems commented Oct 15, 2019

To me this example is a bug and a good reason why default should exist:

{
  "exports": {
    "./": { "node": "./index.mjs", "browser": "./index-browser.mjs" }
  }
}

A runtime that's neither node nor a browser is out of luck here. Yes, the package author could've added an array wrapper and appended a string value as the default implementation. But why should they if this looks more familiar given existing ecosystem practices? I think "every value type has a direct way to specify the default" is a very important invariant. In arrays - add a string. In strings - it's just the string itself. In objects - it's the default key.

@gr2m
Copy link

gr2m commented Oct 15, 2019

For my universal-user-agent example mentioned above other environments are out of luck by definition. But I wouldn't mind adding a "default" key pointing to an entry point that would just return a static string as a fallback.

But I don't see an alternative environment that wouldn't take "browser" as default.

@jkrems
Copy link
Contributor

jkrems commented Oct 15, 2019

But I don't see an alternative environment that wouldn't take "browser" as default.

Deno, D8, or Rhino would be examples of runtimes that are neither. I'm not as familiar with embedded ones, I'm sure there's also some there that don't implement node APIs (especially once we start counting custom embedders for scripting in graphics engines or other tools). Definitely not as big as "browser", especially when counting electron or react-native as browser (?). But they do exist. :)

@ljharb
Copy link
Member

ljharb commented Oct 15, 2019

There is no value in this proposal to me unless it provides a way to allow me to opt in to require and import of the same specifier. The "hazard" some are concerned about almost never occurs in any of my packages, and it's trivial for me to know that anything stateful is a singleton, and "single" means "one", and so i can't duplicate that singleton across a CJS and ESM file without breaking things.

That may not be a compelling enough argument to argue that the "hazard" should be ignored by default, but I think it is compelling to argue that one should be able to opt in to it.

@Jamesernator
Copy link

Jamesernator commented Oct 16, 2019

With TLA can't browser/node split just be done with feature detection anyway?:

// keyVal.js

const DefaultStore
  = typeof window !== 'undefined' ? await import('./indexedDBStore.js')
  : await import('./fileSystemStore.js');

export default class KeyValStore {
  constructor(backingStore=new DefaultStore()) {
    
  }
}

@MylesBorins
Copy link
Contributor

@ljharb while you may not see the issue in your modules multiple large projects have been affected by the problem prior to us removing the behavior in 12.x

graphql/graphql-js#1479
webpack/webpack#7973
facebook/create-react-app#5234

There is no value in this proposal to me

While there may not be value I think it is worth considering that there might be value to other people... and with this feature you could build a custom application level loader that could utilize package meta data to replicate the behavior you want to see happen. I realize this is not as on rails as shipping support directly in Node.js core, but I do think there is still quite a bit of value in this proposal at least in being able to avoid having to do this pattern for any matching

{
  "exports": {
    "./": [{ "browser": "./index-browser.mjs" }, "./index.mjs"]
  }
}

@ljharb
Copy link
Member

ljharb commented Oct 16, 2019

@MylesBorins sure, and that's a fair argument for not allowing it to be the default - but it's not an argument whatsoever for disallowing people to explicitly opt in to the behavior.

@MylesBorins
Copy link
Contributor

@ljharb I'm a bit confused by the way you are using "default". The behavior was never automatic, and in the above cases the module authors explicitly opted in by using the "main": "index" trick. While this may have been implicitly resolved it was still opt-in.

The issue is consumers of modules having an option to opt-out... which you don't really get if this feature is supported by core.

@ljharb
Copy link
Member

ljharb commented Oct 16, 2019

Authors, not consumers, are in charge of how their packages are consumed. Consumers who want to subvert the wishes of authors must - as always - use a transformation tool to do so.

@MylesBorins
Copy link
Contributor

Stepping back for a second.

My point was that there is no such thing as "opt-in" vs "default" in the case here. It is either a capability Node.js supports or doesn't support.

that's a fair argument for not allowing it to be the default - but it's not an argument whatsoever for disallowing people to explicitly opt in to the behavior.

It's either on or off... it is either a hazard in our module system or it isn't. I don't find the line of argument that you are using here very compelling and think framing it as "opt-in" is not a fair way of framing it.

@MylesBorins
Copy link
Contributor

@ljharb FWIW I do think that a path forward here could be shipping an initial implementation of conditional exports with two flags, one with the hazard and one without, in order to gather ecosystem feedback before settling on behavior.

I also want to spend some time with module authors playing with what we are proposing and looking at different transition paths and using that feedback to help guide or design.

While I think this feature is a mistake, I am willing to test the waters. We haven't had as many issues like I've mentioned in #401 (comment) since simplifying the implementation in 12.x... I'm sure that if this hazard is a big deal we will get feedback from developers helping us identify it as a problem.

@guybedford
Copy link
Contributor Author

Conditional exports have now landed on master, with the actual conditional branching of "node" and "require" conditions behind the --experimental-conditional-exports flag.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
modules-agenda To be discussed in a meeting
Projects
None yet
Development

No branches or pull requests

6 participants