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

module: disable conditional exports, self resolve warnings #31845

Closed
wants to merge 3 commits into from
Closed

module: disable conditional exports, self resolve warnings #31845

wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Feb 18, 2020 •
edited
Loading

This removes the experimental warnings for using conditional exports and package self resolution, while keeping these features as experimental.

Currently the warnings are inhibiting usage of conditional exports (see #31819 and discussion at nodejs/modules#485), such that removing them will allow us to get more usage feedback.

At this stage it seems worthwhile to remove the warnings at this point to ensure we have adequate usage and feedback prior to unflagging on the 12.x backport.

We discussed this as a possibility at the last meeting, but it still needs approval from the modules group, so marking as an agenda item there and blocked for landing.

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 c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. labels Feb 18, 2020
@guybedford guybedford added modules-agenda Issues and PRs to discuss during the meetings of the Modules team. pending labels Feb 18, 2020
@guybedford
Copy link
Contributor Author

//cc @nodejs/modules-active-members

@guybedford guybedford mentioned this pull request Feb 18, 2020
Closed
@guybedford
Copy link
Contributor Author

I've gone ahead and also added the commit here to remove the experimental ES module warning.

@guybedford guybedford force-pushed the remove-experimental-exports-warnings branch from 38eef00 to 5c339fd Compare February 18, 2020 19:17
@guybedford guybedford force-pushed the remove-experimental-exports-warnings branch from 5c339fd to 0ce99ad Compare February 18, 2020 22:15
jkrems
jkrems approved these changes Feb 20, 2020
View reviewed changes
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 - does it make sense to split this into two commits (modules vs. resolver feature warnings)?

@aduh95 aduh95 mentioned this pull request Feb 22, 2020
Closed
@mhdawson mhdawson mentioned this pull request Feb 24, 2020
Closed
@guybedford
Copy link
Contributor Author

guybedford commented Feb 26, 2020 •
edited
Loading

I've gone ahead and removed the experimental modules warnings removals from this PR, as per discussion in the meeting.

ljharb
ljharb reviewed Feb 26, 2020
View reviewed changes
lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@guybedford guybedford force-pushed the remove-experimental-exports-warnings branch from 0760a08 to fd2c05b Compare February 26, 2020 20:41
@guybedford guybedford removed modules-agenda Issues and PRs to discuss during the meetings of the Modules team. pending labels Feb 26, 2020
@nodejs-github-bot
Copy link
Collaborator

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

guybedford added a commit that referenced this pull request Feb 26, 2020
@guybedford
module: disable conditional exports, self resolve warnings
49ad161 
PR-URL: #31845
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@guybedford
Copy link
Contributor Author

Landed in 49ad161.

@guybedford guybedford closed this Feb 26, 2020
@guybedford guybedford mentioned this pull request Feb 26, 2020
Closed
4 tasks
codebytere pushed a commit that referenced this pull request Feb 27, 2020
@guybedford @codebytere
module: disable conditional exports, self resolve warnings
b9f3bfe 
PR-URL: #31845
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
Merged
targos pushed a commit that referenced this pull request Apr 18, 2020
@guybedford @targos
module: disable conditional exports, self resolve warnings
85752cd 
PR-URL: #31845
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@transitive-bullshit
Copy link
Contributor

Just tracked this down & wanted to say thanks for all your awesome work @guybedford 🙏😄

@targos
Copy link
Member

targos commented Apr 20, 2020

This lands cleanly on v12.x-staging but the test fails, even if I add --experimental-modules to it. Please backport :)

@guybedford
Copy link
Contributor Author

@targos I suspect this may be because #31757 should land first before this.

@guybedford
Copy link
Contributor Author

Actually sorry that's already landed of course... ok let me look into this.

guybedford added a commit to guybedford/node that referenced this pull request Apr 21, 2020
@guybedford
module: disable conditional exports, self resolve warnings
9ec497a 
PR-URL: nodejs#31845
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@guybedford guybedford mentioned this pull request Apr 21, 2020
Closed
4 tasks
@guybedford
Copy link
Contributor Author

@targos backport created at #32959. Note the changes are just the difference in requiring --experimental-modules, so as expected which is nice!

targos pushed a commit that referenced this pull request Apr 21, 2020
@guybedford @targos
module: disable conditional exports, self resolve warnings
ecddf65 
PR-URL: #31845
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

Backport-PR-URL: #32959
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Apr 22, 2020
Merged
targos added a commit that referenced this pull request Apr 24, 2020
@targos
2020-04-28, Version 12.16.3 'Erbium' (LTS)
2591fb4 
Notable changes:

Dependencies:
  * Updated OpenSSL to 1.1.1f.
    #32583
  * Updated c-ares to 1.16.0.
    #32246
  * Updated experimental uvwasi to 0.0.6.
    #32309
ESM (experimental):
  * Additional warnings are no longer printed for modules that use
    conditional exports or package name self resolution.
    #31845

PR-URL: #33009
targos added a commit that referenced this pull request Apr 27, 2020
@targos
2020-04-28, Version 12.16.3 'Erbium' (LTS)
ada6794 
Notable changes:

Dependencies:
  * Updated OpenSSL to 1.1.1f.
    #32583
  * Updated c-ares to 1.16.0.
    #32246
  * Updated experimental uvwasi to 0.0.6.
    #32309
ESM (experimental):
  * Additional warnings are no longer printed for modules that use
    conditional exports or package name self resolution.
    #31845

PR-URL: #33009
targos added a commit that referenced this pull request Apr 27, 2020
@targos
2020-04-28, Version 12.16.3 'Erbium' (LTS)
63d1e08 
Notable changes:

Dependencies:
  * Updated OpenSSL to 1.1.1g.
    #32971
  * Updated c-ares to 1.16.0.
    #32246
  * Updated experimental uvwasi to 0.0.6.
    #32309
ESM (experimental):
  * Additional warnings are no longer printed for modules that use
    conditional exports or package name self resolution.
    #31845

PR-URL: #33009
targos added a commit that referenced this pull request Apr 28, 2020
@targos
2020-04-28, Version 12.16.3 'Erbium' (LTS)
165011e 
Notable changes:

Dependencies:
  * Updated OpenSSL to 1.1.1g.
    #32971
  * Updated c-ares to 1.16.0.
    #32246
  * Updated experimental uvwasi to 0.0.6.
    #32309
ESM (experimental):
  * Additional warnings are no longer printed for modules that use
    conditional exports or package name self resolution.
    #31845

PR-URL: #33009
@cyungmann cyungmann mentioned this pull request Sep 22, 2020
Closed
@banga banga mentioned this pull request Jul 31, 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

@jkrems jkrems jkrems approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@guybedford @nodejs-github-bot @transitive-bullshit @targos @ljharb @GeoffreyBooth @jkrems