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: throw on require('./path.mjs'); #27417

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
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
Copy link
Collaborator

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

@MylesBorins
Copy link
Contributor Author

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

@MylesBorins MylesBorins mentioned this pull request Apr 25, 2019
Closed
ljharb
ljharb approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

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
Copy link
Contributor Author

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

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

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

@weswigham
Copy link

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

mcollina
mcollina reviewed Apr 25, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

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
Copy link
Member

I’m +1 in landing this in Node 12.0.0

rvagg
rvagg approved these changes Apr 26, 2019
View reviewed changes
Copy link
Member

@rvagg rvagg left a comment

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
Copy link
Collaborator

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

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
Copy link
Contributor Author

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

@sam-github
Copy link
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-github sam-github mentioned this pull request May 1, 2019
Closed
@nodejs-github-bot
Copy link
Collaborator

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

Trott
Trott approved these changes May 1, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

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 MylesBorins force-pushed the module-breaking-change branch 2 times, most recently from beb834e to 62fe651 Compare May 1, 2019 07:46
@MylesBorins
Copy link
Contributor Author

added basic test + doc ptal

@nodejs-github-bot
Copy link
Collaborator

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

targos
targos reviewed May 1, 2019
View reviewed changes
doc/api/modules.md Outdated Show resolved Hide resolved
doc/api/modules.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@MylesBorins
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@Trott Trott 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
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

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

latest CI was green

landed in d370d12

@MylesBorins MylesBorins closed this May 3, 2019
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>
@targos targos 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
@coreyfarrell coreyfarrell mentioned this pull request Sep 26, 2019
Open
@aduh95 aduh95 mentioned this pull request Oct 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina left review comments

@rvagg rvagg rvagg approved these changes

@jkrems jkrems jkrems approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ljharb ljharb ljharb approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@inidaname inidaname inidaname approved these changes

@devsnek devsnek devsnek approved these changes

@robpalme robpalme robpalme approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.