-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
esm: revert format name to "cjs" over "commonjs" #18596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We should fast track
PR-URL: nodejs#18596 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Landed in 93df116 |
PR-URL: #18596 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Should this be backported to |
This should definitely be backported for consistency. Sure will work on this. |
Actually this only needs to be backported if #16874 is backported. |
PR-URL: nodejs#18596 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This reverts the change originally made in #16874, returning the "format" name to
"cjs"
instead of"commonjs"
.There was some complaint over consistency of the new name form, so if we do want to change this should aim to get it in before that gets released.
Note the documentation change has already been released here, while the code change hasn't yet.
//cc @devsnek @MylesBorins @targos @jdalton
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
esm