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

[v12.x] Backport unflag --experimental-modules #33055

Closed
wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Apr 25, 2020

/cc @nodejs/modules-active-members @nodejs/lts

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Apr 25, 2020
@targos
Copy link
Member Author

targos commented Apr 25, 2020

This is to prepare the next v12.x minor, which is scheduled for 2020-05-26

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Does this include removal of the warning, and any other ESM bugfixes that were part of v13/v14?

if (!this.exportKeys) {
// When using --expose-internals, we do not want to reflect the named
// exports from core modules as this can trigger unnecessary getters.
const internal = this.id.startsWith('internal/');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const internal = this.id.startsWith('internal/');
const internal = StringPrototypeStartsWith(this.id, 'internal/');

i realize this is a backport PR, so maybe it should land in master with this change first, but it seems important to avoid brittleness in LTS specifically.

@targos
Copy link
Member Author

targos commented Apr 25, 2020

This doesn't include the removal of the warning.
Many of the fixes are already on v12.x. This is the only PR that we did not backport in previous releases.

@ljharb
Copy link
Member

ljharb commented Apr 25, 2020

Do we want it unflagged on LTS, without the warning? We removed the warning in v13 because it was inhibiting adoption; i think it would have that effect much more on an LTS version.

@targos
Copy link
Member Author

targos commented Apr 25, 2020

I did not include the removal of the warning in this PR but I intend to cherry pick it as well later

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 26, 2020
@MylesBorins
Copy link
Contributor

Please open removing the warning as a separate PR. I'm not sure that we should remove the warning on LTS prior to 14.x moving to LTS

@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented Apr 26, 2020

@MylesBorins let's discuss in the next modules meeting; i'm not sure we should remove the flag without removing the warning.

@targos targos added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Apr 26, 2020
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 26, 2020 via email

@richardlau
Copy link
Member

A question I have is does removing the flag potentially cause an observable change in behaviour if, say, packages had defined fields such as described/warned about in #33074? Or do package.json files with esm fields already get loaded as esm on 12.x without the experimental flag (I was under the impression they weren’t)?

@ljharb
Copy link
Member

ljharb commented Apr 26, 2020

i believe any package that has opted in to use conditional exports, for example, will suddenly start to generate a warning in 12 if this lands without removing the warning - to my recollection, that experience is a big part of why we removed the warning in 13.

@richardlau
Copy link
Member

My question was more around for anyone who is not currently opting in to esm but may be using dependencies that might start behaving differently once the flag is removed.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@SimenB
Copy link
Member

SimenB commented May 4, 2020

Landing this will also close #33202 I guess 🙂

guybedford and others added 2 commits May 6, 2020 21:17
targos pushed a commit that referenced this pull request May 7, 2020
PR-URL: #29866
Backport-PR-URL: #33055
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos pushed a commit that referenced this pull request May 7, 2020
PR-URL: #30720
Backport-PR-URL: #33055
Reviewed-By: Guy Bedford <[email protected]>
@targos
Copy link
Member Author

targos commented May 7, 2020

Landed on v12.x-staging

@targos targos closed this May 7, 2020
@targos targos deleted the unflag-esm-v12 branch May 7, 2020 07:00
targos pushed a commit that referenced this pull request May 13, 2020
PR-URL: #29866
Backport-PR-URL: #33055
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos pushed a commit that referenced this pull request May 13, 2020
PR-URL: #30720
Backport-PR-URL: #33055
Reviewed-By: Guy Bedford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. modules-agenda Issues and PRs to discuss during the meetings of the Modules team. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.