Skip to content

module: throw on require('./path.mjs'); - #27417

Closed
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:module-breaking-change
Closed

module: throw on require('./path.mjs');#27417
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:module-breaking-change

Conversation

@MylesBorins

@MylesBorinsMylesBorins commented Apr 25, 2019

Copy link
Copy Markdown
Contributor

This is an extremely important part of the ESM implementation
that should have been unflagged as a breaking change in v12.0.0
to allow us to unflag ESM in Node.js 12.x before LTS. Assuming we
can get consensus on this behavior I would argue that this Semver-Major
behavior change could be viewed as a Semver-Patch fix in v12.0.1

Thoughts?

/cc @nodejs/modules @nodejs/tsc

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2019

Copy link
Copy Markdown
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3367

@MylesBorins

MylesBorins commented Apr 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Alternatively... if this is not viewed as a breaking change (adding throw to require) we can punt on this until later. I think it likely is though, in which case we should try and get this landed + out in the wild ASAP if we can reach consensus

@MylesBorinsMylesBorins mentioned this pull request Apr 25, 2019
Closed
ljharb
ljharb approved these changes Apr 25, 2019
View reviewed changes

@ljharbljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anybody doing anything useful with requiring an .mjs extension, without the flag, would have had to install their own require.extensions handler. If they've done that, this won't break them. If they haven't, requiring an .mjs file would fail to parse anything with import or export in it, and throw, which I would assume is the common case.

Thus, while this is technically breaking, imo it seems very very safe, and I agree with the logic that this was a bug in the ESM implementation.

@MylesBorins

MylesBorins commented Apr 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Also, this requires tests / docs. Will update assuming we have consensus to make this change

@MylesBorinsMylesBorins added the wip Issues and PRs that are still a work in progress. label Apr 25, 2019
@MylesBorins

MylesBorins commented Apr 25, 2019

Copy link
Copy Markdown
ContributorAuthor

A qq. If we add this, would removing it in the future be Semver-Patch or Semver-Major?

@weswigham

weswigham commented Apr 25, 2019

Copy link
Copy Markdown

Removing an error is normally considered semver patch, right? Since it's just an enhancement?

mcollina
mcollina reviewed Apr 25, 2019
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should add a unit test as well.

@mcollina

mcollina commented Apr 25, 2019

Copy link
Copy Markdown
Member

I’m +1 in landing this in Node 12.0.0

rvagg
rvagg approved these changes Apr 26, 2019
View reviewed changes

@rvaggrvagg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everyone gets a .1 for this kind of thing

is there a reference to a discussion or modules roadmap that has this in it that can help with justification?

@nodejs-github-bot

nodejs-github-bot commented Apr 30, 2019•
edited by MylesBorins
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22839/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1841

@MylesBorins

MylesBorins commented Apr 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Running CI + CITGM. If things look good I'm going to document change and then land... eta 48 hours

@sam-github

sam-github commented May 1, 2019

Copy link
Copy Markdown
Contributor

I will resume this, despite what the log looks like, I think what actually failed is jenkins. Because the java stack got inserted into the ld command output, it looks like the "Exported symbol not defined" is fatal, but I checked out the passed AIX builds, and they have the same message. I think there should be a newline before the FATAL below, and that its the java that failed.

ld: 0711-319 WARNING: Exported symbol not defined: v8:FATAL: command execution failed
java.nio.channels.ClosedChannelException
at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:209)
at org.jenkinsci.remoting.protocol.ApplicationLayer.onRecvClosed(ApplicationLayer.java:222)
at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:816)

@sam-githubsam-github mentioned this pull request May 1, 2019
Closed
@nodejs-github-bot

nodejs-github-bot commented May 1, 2019

Copy link
Copy Markdown
Collaborator

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

Trott
Trott approved these changes May 1, 2019
View reviewed changes

@TrottTrott left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if a test and documentation are added.

@MylesBorins
MylesBorinsforce-pushed the module-breaking-change branch 2 times, most recently from beb834e to 62fe651CompareMay 1, 2019 07:46
@MylesBorins

MylesBorins commented May 1, 2019

Copy link
Copy Markdown
ContributorAuthor

added basic test + doc ptal

@nodejs-github-bot

nodejs-github-bot commented May 1, 2019

Copy link
Copy Markdown
Collaborator

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

targos
targos reviewed May 1, 2019
View reviewed changes
Comment threaddoc/api/modules.md Outdated
Comment threaddoc/api/modules.md Outdated
@nodejs-github-bot

nodejs-github-bot commented May 2, 2019

Copy link
Copy Markdown
Collaborator

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

@MylesBorins

MylesBorins commented May 2, 2019

Copy link
Copy Markdown
ContributorAuthor

@Trott I took all your suggestions and tweaked a bit more. Rebased the branch to clean up the noise.

Feel free to edit away and push on the branch.

@nodejs-github-bot

nodejs-github-bot commented May 3, 2019

Copy link
Copy Markdown
Collaborator

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

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2019
@nodejs-github-bot

nodejs-github-bot commented May 3, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 3, 2019

Copy link
Copy Markdown
Collaborator

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

@MylesBorinsMylesBorins added notable-change PRs with changes that should be highlighted in changelogs. dont-land-on-v8.x labels May 3, 2019
@MylesBorins

MylesBorins commented May 3, 2019

Copy link
Copy Markdown
ContributorAuthor

latest CI was green

landed in d370d12

MylesBorins added a commit that referenced this pull request May 3, 2019
@MylesBorins
module: throw on require('./path.mjs');
d370d12
This is an extremely important part of the ESM implementation
that should have been unflagged as a breaking change in v12.0.0
to allow us to unflag ESM in Node.js 12.x before LTS. Assuming we
can get consensus on this behavior I would argue that this Semver-Major
behavior change could be viewed as a Semver-Patch fix in v12.0.1
PR-URL: #27417
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2019
@MylesBorins@targos
module: throw on require('./path.mjs');
014a9fd
This is an extremely important part of the ESM implementation
that should have been unflagged as a breaking change in v12.0.0
to allow us to unflag ESM in Node.js 12.x before LTS. Assuming we
can get consensus on this behavior I would argue that this Semver-Major
behavior change could be viewed as a Semver-Patch fix in v12.0.1
PR-URL: #27417
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targostargos mentioned this pull request May 6, 2019
Merged
targos added a commit that referenced this pull request May 6, 2019
@targos
2019-05-07, Version 12.2.0 (Current)
492762e
Notable changes:
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function a file URL object, a file URL string or an absolute path
string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
targos added a commit that referenced this pull request May 7, 2019
@targos
2019-05-07, Version 12.2.0 (Current)
e8dda76
Notable changes:
* deps:
* Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
parser refuse any request URL that contained the "|" (vertical bar)
character. #27595
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497#27376
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function from a file URL object, a file URL string or an absolute
path string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* repl:
* The REPL now supports multi-line statements using `BigInt` literals
as well as public and private class fields and methods.
#27400
* The REPL now supports tab autocompletion of file paths with `fs`
methods. #26648
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
targos added a commit that referenced this pull request May 7, 2019
@targos
2019-05-07, Version 12.2.0 (Current)
e006a85
Notable changes:
* deps:
* Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
parser refuse any request URL that contained the "|" (vertical bar)
character. #27595
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497#27376
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function from a file URL object, a file URL string or an absolute
path string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* repl:
* The REPL now supports multi-line statements using `BigInt` literals
as well as public and private class fields and methods.
#27400
* The REPL now supports tab autocompletion of file paths with `fs`
methods. #26648
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
This was referenced May 7, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@coreyfarrellcoreyfarrell mentioned this pull request Sep 26, 2019
Closed
@aduh95aduh95 mentioned this pull request Oct 24, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@mcollinamcollinamcollina left review comments
@targostargostargos approved these changes
@hybristhybristhybrist approved these changes
@ljharbljharbljharb approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
@BridgeARBridgeARBridgeAR approved these changes
@jasnelljasnelljasnell approved these changes
+6 more reviewers
@thefourtheyethefourtheyethefourtheye approved these changes
@rvaggrvaggrvagg approved these changes
@inidanameinidanameinidaname approved these changes
@devsnekdevsnekdevsnek approved these changes
@robpalmerobpalmerobpalme approved these changes
@ryzokukenryzokukenryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.notable-changePRs with changes that should be highlighted in changelogs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

20 participants

@MylesBorins@nodejs-github-bot@weswigham@mcollina@sam-github@Trott@ljharb@jasnell@rvagg@hybrist@thefourtheye@benjamingr@targos@cjihrig@tniessen@inidaname@devsnek@robpalme@BridgeAR@ryzokuken