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 condition renaming proposal for usability #30799

Closed
wants to merge 8 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Dec 5, 2019

A major priority for the modules implementation is resolver stability, and the dual mode story through conditional exports is a big remaining piece of this.

Common usability feedback out of various discussions on conditional exports so far has been that the "default" field may be seen to be a confusing name, and that it isn't clear when the "require" condition will match either.

To try and improve the overall usability this PR makes the following condition name changes:

  • Add a new "import" condition as the converse of the "require" condition, only applying for the ESM loader.

All conditions (except for "default") remain behind the --experimental-conditional-exports flag.

This makes the dual mode workflow look like:

{
  "type": "module",
  "main": "./index.cjs",
  "exports": {
    "require": "./index.cjs",
    "import": "./index.js"
  }
}

instead of the previous:

{
  "type": "module",
  "main": "./index.cjs",
  "exports": {
    "require": "./index.cjs",
    "default": "./index.js"
  }
}

the UX improvement being that the former seems like it will look more natural to most users unfamiliar with "exports".

Modules group discussion in nodejs/modules#452, which this PR can be considered blocked on for now.

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 Dec 5, 2019
@guybedford guybedford added blocked PRs that are blocked by other issues or PRs. esm Issues and PRs related to the ECMAScript Modules implementation. labels Dec 5, 2019
@ljharb
Copy link
Member

ljharb commented Dec 6, 2019

I mentioned this on the modules repo thread but it seems to have been ignored, so I'm posting this here to be sure we make decisions with eyes open. (I'm happy to discuss this in a different medium if this isn't the best venue, but i think it's important to surface it here)


Removing the "default" field will break existing packages' non-main exports when they exist, and their main exports when they used the object form without array fallback notation. A specific example (i have multiple; eg i've created 4 new packages in the last week, all with "exports", to add to existing highly used dep graphs like deep-equal and resolve and tape): https://npmcdn.com/[email protected]/package.json

Assuming 13.4 is what includes this PR: 1. node >= v13.4: `require('es-get-iterator/package')` and `require('es-get-iterator/package.json')` break; the 13.0/13.1 string fallback continues to work for the "main" 1. node v13.2.x - v13.3.x: unchanged ofc; `require('es-get-iterator')` and `require('es-get-iterator/package')` and `require('es-get-iterator/package.json')` all continue to work. 1. node v13.0.x - v13.1.x: unchanged ofc; these break on the object syntax, and will fall back to the "main" entry point. They are already unable to `require('es-get-iterator/package')` and `require('es-get-iterator/package.json')` due to bugs in "exports". 1. node v12.x, assuming no backport: unchanged: these can require any file inside the package. 1. node v12.x, assuming a backport: `require('es-get-iterator/package')` and `require('es-get-iterator/package.json')` are broken, and what worked in an LTS node stops working.

The first item can be technically considered acceptable for an experimental feature in node 13, altho it's highly unfortunate and inconvenient.

The last item, however, strikes me as something that an LTS policy doesn't/shouldn't allow (correct me if i'm wrong on the former). Thus, removing "default" entirely seems like it would prevent backporting to v12 (without restoring it somehow for the backport).

Is there any way we could keep "default" for now, and remove it as part of v14, rather than breaking (arguably, capriciously) consumers of these packages?

(Note that I can and certainly will publish a patch for affected packages that adds "require" alongside "default"; this is a solution for anyone who isn't expecting to be forced to update their lockfile on a non-major upgrade of node, which seems like "nobody")

@GeoffreyBooth
Copy link
Member

Package exports are experimental, and "default" has only been in the wild since 2019-11-21. If we’re going to remove it, better to do so as soon as possible before usage increases any further.

Anyone who uses experimental features in production assumes the risks of breaking changes. For the sake of downstream users, it would be advisable not to use experimental features in stable versions of public packages.

@ljharb
Copy link
Member

ljharb commented Dec 6, 2019

It's experimental in node 13; it's "nothing" in node 12. The primary risk I'm talking about is future backport risk for v12. (also, everything published is stable; semver communicates breakage not stability - if we want feedback, these features have to be used in published stable packages).

My understanding of the deferring of conditional exports was about the other keys, not about changing "default".

@guybedford guybedford removed the blocked PRs that are blocked by other issues or PRs. label Dec 9, 2019
@guybedford
Copy link
Contributor Author

It would be great to get this one landed... @jkrems @MylesBorins @nodejs/modules-active-members, please review if you can.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2019

@guybedford we haven't discussed it in the modules meeting yet, and it seems like my very real concerns aren't being considered or addressed. Please do not rename "default" casually.

@guybedford
Copy link
Contributor Author

@ljharb this PR doesn't remove or rename "default", rather it supports "import" alongside "default" to ensure that users can provide a more readable "import" / "require" pattern. Any discussion over deprecation of "default" would be separate and is out of scope for this. I'm happy either way on that question personally.

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.

LGTM in general, two questions.

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

jkrems commented Dec 9, 2019

Any discussion over deprecation of "default" would be separate and is out of scope for this. I'm happy either way on that question personally.

I think it's fair to call this a "doc only deprecation of the default condition" though..?

@ljharb
Copy link
Member

ljharb commented Dec 9, 2019

@guybedford thank you very much for clarifying; that means "default" won't break, which is great. Will "import" and/or "require" take precedence over "default" if all three are specified?

@jkrems doc-only is something i'm 100% fine with, ftr :-)

@guybedford
Copy link
Contributor Author

@ljharb yes "import" and "require" both have a higher precedence than default. And "node" has a higher precedence than both of those as well. "default" is always the lowest precedence of all future conditions too.

@ljharb
Copy link
Member

ljharb commented Dec 9, 2019

Wonderful, thanks for clarifying. My concerns are addressed.

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

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

I left a note about including default in the resolution docs. I'm personally not a fan of deprecating that entry and I think it should be officially documented.

doc/api/esm.md Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

guybedford commented Dec 10, 2019

So one problem we have with this PR landing - is that currently we can recommend the "default" pattern to users without them using the --experimental-conditional-exports flag, whereas this pattern will not work at all unless the flag is set.

For this reason I think we should revert the documentation on this PR to still use the "require" / "default" pattern, and then instead we can change the documentation to recommend the "import" / "require" pattern when we unflag conditional exports.

The next risk as well with this pattern is that it will break on the current 13 versions that there is no valid "exports" resolution.

Perhaps that does mean we are too late to make this change after all?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2019

@guybedford it seems like packages could use this pattern to work on every minor version of node i'm aware of:

      "exports": {
		".": [
			{
				"import": "./main.mjs",
				"require": "./main.js"
			},
			{
				"default": "./main.js"
			},
			"./main.js"
		]
	},

(perhaps the two objects could be combined; i'd have to test it across node versions once the conditional changes are available)

although I like the idea of the docs recommending the "unflagged" pattern instead of a flagged one, I think that this change - given node 13's "experimental" status, and given that a backport to node 12 would hopefully include unflagged conditional exports - seems like a reasonable back compat story to me, which means we could continue with the renaming.

@guybedford
Copy link
Contributor Author

Thanks @ljharb for the thoughts on this.

Admittedly the dual mode pattern as we are advocating does break when using CJS require itself though in 13 currently (cannot require module at "default"), so it's not as if we're recommending a pattern that doesn't already cause breaking workflows for users since users shipping dual mode support like the docs mention will already be breaking all their users using require() on Node.js 13.x.

Perhaps we should just clarify these patterns are likely to break until they are unflagged?

My worry with this is if these patterns break now, how can we ever encourage people use them if they simply can't work.

@guybedford
Copy link
Contributor Author

On that note, @ljharb if you are shipping "exports" have you tested require() in Node.js 13?

@guybedford
Copy link
Contributor Author

(spoiler, it breaks :P)

@ljharb
Copy link
Member

ljharb commented Dec 10, 2019

I’m shipping “exports” with a dot set to default with a string fallback, and testing it in every node minor since 0.6 (es-get-iterator, via its dependents) and afaict it works in every one.

@guybedford
Copy link
Contributor Author

guybedford commented Dec 10, 2019

Specifically though, did you test require('pkg') in Node.js 13.1 13.2?

@ljharb
Copy link
Member

ljharb commented Dec 10, 2019

Yes, and it works if my tests are to be believed. Note that I’m talking about CJS only; i won’t ship ESM anywhere until conditionals are unflagged :-)

@guybedford
Copy link
Contributor Author

Ah right, that works yes.

@guybedford
Copy link
Contributor Author

Thanks for working this through with me here.... just helps to have some feedback on the compat implications.

My main concern is just that we have enough guidance here to note that setting a "require" or "import" condition in the package.json will result in packages that will be highly unstable and likely to break. I know we have a note on this, but I don't think we are stressing it enough.

I've added another warning to the docs, and kept the overall workflows the same for this PR since it's breaks either way.

Further suggestions welcome, otherwise I think this is good to land.

@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Dec 12, 2019
PR-URL: #30799
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@guybedford
Copy link
Contributor Author

Landed in 357a992.

@guybedford guybedford closed this Dec 12, 2019
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
PR-URL: #30799
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
PR-URL: #30799
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #30799
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants