module: move cjs type check behind experimental-modules flag #29732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
//cc @nodejs/modules-active-members |
targos
approved these changes
Sep 27, 2019
devsnek
reviewed
Sep 27, 2019
devnexen
approved these changes
Sep 27, 2019
gengjiawen
approved these changes
Sep 27, 2019
cjihrig
approved these changes
Sep 27, 2019
jkrems
approved these changes
Sep 27, 2019
guybedford
added
author ready
|
I'd like to suggest we fast-track this change as much as possible too. |
guybedford
added
the
fast-track
devsnek
approved these changes
Sep 27, 2019
|
one more checkmark from a core collab from after the fast track label and this can be fast tracked |
jkrems
approved these changes
Sep 27, 2019
|
I treated the comment as the fast-tracking request but I also added the checkmark. So I think this can land now! |
jasnell
approved these changes
Sep 27, 2019
antsmartian
approved these changes
Sep 28, 2019
|
CI is failing because of an unrelated issue. To unblock this, go over to #29741 (comment) and approve the PR and fast-tracking. |
|
Landed in 2825e11 |
Trott
pushed a commit
that referenced
this pull request
Sep 28, 2019
PR-URL: #29732 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
targos
pushed a commit
that referenced
this pull request
Oct 1, 2019
PR-URL: #29732 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
This was referenced Oct 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This puts the check added in #29492 behind the
--experimental-modulesflag instead of being on by default.We added this error for require(.js) inside of "type": "module" packages to reduce an edge case hazard but it has caused more compatibility issues itself, for which we need to discuss a way forward.
While we do that, at least restricting the error to
--experimental-modulesusage will avoid unintentional compatibility issues unflagged.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes