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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

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

ljharb
ljharb reviewed Apr 25, 2020
View reviewed changes
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?

lib/internal/bootstrap/loaders.js
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
Member

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

CI: https://ci.nodejs.org/job/node-test-pull-request/30986/

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

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.

@SimenB SimenB mentioned this pull request Apr 27, 2020
Closed
@aduh95 aduh95 mentioned this pull request Apr 29, 2020
Closed
@mhdawson mhdawson mentioned this pull request May 4, 2020
Closed
mcollina
mcollina approved these changes May 4, 2020
View reviewed changes
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
@guybedford @targos
esm: unflag --experimental-modules
e7ba623 
PR-URL: nodejs#29866
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@juanarbol @targos
lib: add warning on dynamic import es modules
c16d4da 
PR-URL: nodejs#30720
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request May 7, 2020
@guybedford @targos
esm: unflag --experimental-modules
f9edab1 
PR-URL: #29866
Backport-PR-URL: #33055
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit that referenced this pull request May 7, 2020
@juanarbol @targos
lib: add warning on dynamic import es modules
233b988 
PR-URL: #30720
Backport-PR-URL: #33055
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@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
@aduh95 aduh95 mentioned this pull request May 7, 2020
Closed
targos pushed a commit that referenced this pull request May 13, 2020
@guybedford @targos
esm: unflag --experimental-modules
93226a5 
PR-URL: #29866
Backport-PR-URL: #33055
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit that referenced this pull request May 13, 2020
@juanarbol @targos
lib: add warning on dynamic import es modules
698e21b 
PR-URL: #30720
Backport-PR-URL: #33055
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@targos targos mentioned this pull request Jan 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@mcollina mcollina mcollina approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet