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

doc: module: Conditional exports docs for writing dual packages #30051

Closed

Conversation

@GeoffreyBooth
Copy link
Contributor

GeoffreyBooth commented Oct 21, 2019

This PR is an addendum to @guybedford’s #29978. This adds documentation to explain what the divergent specifier hazard is and provide two approaches for preventing or avoiding it when writing dual CommonJS/ES module packages. This came out of discussion in nodejs/modules#371 including some suggestions from @coreyfarrell. cc @MylesBorins.

This PR includes the other one, so please focus on esm.md starting at the “Dual CommonJS/ES Module Packages” heading: https://github.com/nodejs/node/pull/30051/files?short_path=8e67f40#diff-8e67f407bc32a0569e25d7ecaff6e494

Checklist
@GeoffreyBooth GeoffreyBooth changed the title module: Conditional exports docs for writing dual packages doc: module: Conditional exports docs for writing dual packages Oct 21, 2019
@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Oct 21, 2019

Copy link
Contributor

coreyfarrell left a comment

Overall I like this, thank you.

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

This comment has been minimized.

Copy link
Contributor Author

GeoffreyBooth commented Oct 23, 2019

@coreyfarrell and @ljharb I’ve revised per your notes, please let me know what you think. (And please check my example code.)

Would appreciate more reviews from anyone else interested in dual packages.

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

This comment has been minimized.

Copy link

weswigham commented Oct 23, 2019

FWIW right now the primary reccomdation to avoid any sort of copy hazard should really just be to ship and use cjs. Until an es module can be used as a replacement for a cjs one and one can get away with only shipping that, it'll always exist in some form or another. The real issue is attempting to encourage shipping two implementations that could be accessed at all... It's just not a great idea, from the perspective of the copy hazard. If you acknowledge that but still want to encourage shipping multiple implementations, then it's a question of where you want to land on how easy it is to accidently introduce a duplicated state issue. I'm sure someone is screaming internally that using Symbol.hasInstance to patch around one aspect of the hazard is treacherous enough. If you're cognizant enough to consider such a fix in your library, why would you not just ship an authorative cjs source that's reexported thru and esm facade? That's much lower maintenance and way less error prone...

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

GeoffreyBooth commented Oct 23, 2019

why would you not just ship an authorative cjs source that’s reexported thru and esm facade?

That’s the first approach that this PR recommends. The issue is that a lot of people won’t consider that “good enough”—it’s still a CommonJS package, just with a sheen of ESM slapped over it. People want to publish pure ESM packages, and they don’t want to drop CommonJS support. Hence approach 2, which I’m trying to go into detail laying out all its disadvantages and gotchas so that people might give it a second thought before attempting it. I feel like if we don’t mention approach 2 at all and don’t mention all the hoops you need to jump through to try to make it avoid hazards, people will just do it anyway and ship all the bugs we’re hoping they avoid.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 23, 2019

People want to publish pure ESM packages, and they don’t want to drop CommonJS support.

if they're shipping a cjs source, they're shipping a cjs source. We should have one authoritative source for each module. Even mentioning hasInstance is a huge red flag to me, it's horrifying.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Oct 23, 2019

I think there's various author vs. package vs. consumer questions that we can't answer all at once. Some authors really want to maintain their source of truth as ESM but aren't ready to drop CJS support (or are they?). Some packages (many packages?) aren't stateful and could just work with two instances. We should be careful to set a very clear scope in terms of audience and problem space. Otherwise we could write a whole book and still only scratch the surface. :)

Even mentioning hasInstance is a huge red flag to me, it's horrifying.

Agreed. I had hoped nobody would think of Symbol.hasInstance. I don't mind it being mentioned as an advanced feature with a link to MDN but I don't believe we should give people a play-by-play of using it for this.

@coreyfarrell

This comment has been minimized.

Copy link
Contributor

coreyfarrell commented Oct 23, 2019

Even mentioning hasInstance is a huge red flag to me, it's horrifying.

Agreed. I had hoped nobody would think of Symbol.hasInstance. I don't mind it being mentioned as an advanced feature with a link to the docs but I don't believe we should give people are play-by-play of using it for this.

Sorry. I was looking for a solution not just for this but for duplicated installs. I think linking to the MDN documentation without any further context will result in implementations that fail to correctly deal with inheritance. It took me a few iterations to correctly deal with inheritance in hasInstance and I only found that it was an issue it because I intentionally tested inheritance.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

GeoffreyBooth commented Oct 23, 2019

I had hoped nobody would think of Symbol.hasInstance. I don’t mind it being mentioned as an advanced feature with a link to the docs but I don’t believe we should give people are play-by-play of using it for this.

The counterargument would be that if we don’t give people a recommended way of doing it, they’ll likely use suboptimal solutions that have bugs. That’s really the point of approach 2 in general: we acknowledge that people want to do this and will try to do this, so if they do, we might as well tell them the best possible way to do it and point out the gotchas that are still present even after following all our advice. I think that’s better than forbidden knowledge where users are on their own to figure it out, and they almost certainly won’t do as good a job as we could.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 23, 2019

@GeoffreyBooth 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

This comment has been minimized.

Copy link
Contributor Author

GeoffreyBooth commented Oct 23, 2019

i think it would be better for the ecosystem to just not support this for the time being.

That’s the argument for not supporting conditional exports at all (and therefore not permitting the hazard). That’s not part of this PR. This PR is an addendum to that one; if that one is merged in, and therefore we permit the hazard, what should the docs say to advise users how to avoid bugs? If that PR never gets merged in then this one is moot.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 23, 2019

@GeoffreyBooth then it seems i should post this on the other pr too

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Oct 23, 2019

The counterargument would be that if we don’t give people a recommended way of doing it, they’ll likely use suboptimal solutions that have bugs.

I think there's ways to do it that we can recommend that are easier to get right. E.g. marking each instance with a symbol. Or putting the class implementations into CJS files and refer to them from both variants. Symbol.hasInstance is very hard to get right as you experienced yourself and people will make mistakes while copying the solution.

It also comes with a "voids warranty" on JS engine performance and, imo, make code much harder to debug because you can suddenly have an instanceof check that succeeds without the class being anywhere in the prototype chain. And there's no obvious sign that this magic is going on.

i think it would be better for the ecosystem to just not support this for the time being.

I think the simple question is: How? We can't prevent people from shipping pkg and pkg/esm and it solves their problem. So how do we not support it? If that pattern spreads, we'll have to somehow address the issues that come from it. I'm not seeing what our alternative is.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 23, 2019

@jkrems pkg and pkg/esm is a separate concern. you've imported two different specifiers, you get two different modules.

@devsnek devsnek referenced this pull request Oct 23, 2019
4 of 4 tasks complete
@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

GeoffreyBooth commented Oct 23, 2019

I removed the references to Symbol.hasInstance for now. We can add that later if that seems like a solution we want to recommend. 9a620e3

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Oct 23, 2019

pkg and pkg/esm is a separate concern. you've imported two different specifiers, you get two different modules.

You may not have. You may have imported N different packages using various specifiers, some of which using pkg, some using pkg/esm. Depending on how each is implemented.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 23, 2019

@jkrems if you think the pkg/module and pkg situation is bad, why on earth would you want to add another implicit way for that to happen

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Oct 23, 2019

if you think the pkg/module and pkg situation is bad, why on earth would you want to add another implicit way for that to happen

This isn't implicit. It's explicit. The package author (the person who's most likely to know the kinds of issues that may arise from multiple instances) is explicitly saying "if you use these two copies in the same app, it'll be fine"

That's making things better! Because people will ship two copies. If they are passionate about ESM (many are), they will encourage their users to actually use the ESM copy. This gives them a safer path to achieve this, much safer than saying "just ignore those issues and publish both versions under separate specifiers, surely no package tree will ever lead to inconsistencies!".

This is about where to put the responsibility when (not if) a package includes both versions. Without this feature, the responsibility is always shared across all consumers in the package tree. That's an awful situation imo. With this feature, the responsibility can be placed in a single, isolated codebase: The package itself. I believe that's way better than the status quo.

@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:exports-conditions-warnings branch from 9a620e3 to 3f055a8 Oct 31, 2019
@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor Author

GeoffreyBooth commented Oct 31, 2019

There was consensus in the 2019-10-30 modules meeting that this PR (or something like it) should land as part of landing conditional exports (which was also approved to land with some changes per the group, see #29978 (comment)). @Trott and others, do you mind please taking a look at this PR?

This PR includes the other one, so please focus on esm.md starting at the “Dual CommonJS/ES Module Packages” heading: https://github.com/nodejs/node/pull/30051/files?short_path=8e67f40#diff-8e67f407bc32a0569e25d7ecaff6e494. esm.md is the only file changed in this PR relative to #29978.

Copy link
Contributor

guybedford left a comment

This is looking really great, the flow of the material works really well and it is very clear now, thanks.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
Copy link
Contributor

guybedford left a comment

Good to land with the conditional exports PR.

@guybedford guybedford force-pushed the GeoffreyBooth:exports-conditions-warnings branch from 3f055a8 to ee79a18 Oct 31, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Oct 31, 2019

I've pushed up a commit to remove the section on global state, as well as noting default exports in the wrapper. Re-review welcome.

Copy link
Member

ljharb left a comment

Looks great overall.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford guybedford force-pushed the GeoffreyBooth:exports-conditions-warnings branch from 311a8e1 to 8de673d Nov 1, 2019
@jkrems
jkrems approved these changes Nov 1, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 1, 2019

This PR seems ready to go, unless there is further feedback on any of the wording here.

I've added the blocked label added as this must land alongside #29978.

@addaleax addaleax removed the author ready label Nov 5, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 7, 2019

I've added an extra commit here to note the --experimental-conditional-exports flag and sugar.

@nodejs-github-bot

This comment has been minimized.

@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 8, 2019

I've run this CI multiple times, but there is an out of memory error just on Docker arm v7 on the build itself which seems to be entirely unrelated.

Edit: this comment applies to #29978 not this PR.

@guybedford guybedford force-pushed the GeoffreyBooth:exports-conditions-warnings branch from ec15f17 to a944456 Nov 8, 2019
guybedford added a commit that referenced this pull request Nov 8, 2019
PR-URL: #30051
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 8, 2019

Lite CI passing - landed in ebe3dcc.

@guybedford guybedford closed this Nov 8, 2019
@ljharb ljharb deleted the GeoffreyBooth:exports-conditions-warnings branch Nov 8, 2019
@GeoffreyBooth GeoffreyBooth referenced this pull request Nov 9, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.