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: disable conditional exports, self resolve warnings #31845

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Feb 18, 2020

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
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
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)?

@guybedford
Copy link
Contributor Author

guybedford commented Feb 26, 2020

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

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

@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

guybedford added a commit that referenced this pull request Feb 26, 2020
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
codebytere pushed a commit that referenced this pull request Feb 27, 2020
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
targos pushed a commit that referenced this pull request Apr 18, 2020
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
PR-URL: nodejs#31845
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@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
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 added a commit that referenced this pull request Apr 24, 2020
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
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
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
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
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.

None yet

7 participants