module: remove require('.') with NODE_PATH compat#3384

silverwind · 2015-10-15T12:57:10Z

This PR replaces #1452. It's updated and targets master now. Adding 6.0.0 milestone as discussed.

silverwind · 2015-10-15T13:03:35Z

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

silverwind · 2015-12-01T16:36:25Z

Going to postpone this removal once more. This incompatibilty and the subsequent hack were added during io.js. the exposure time of this deprecation message would be quite short for users of 4.x. Likely target is 7.0.0.

jasnell · 2016-03-22T05:52:59Z

@silverwind ... is this still something you want to pursue?

silverwind · 2016-03-22T12:14:58Z

Yes, I'm just waiting for the v6 release so I can land this on master.

benjamingr · 2016-04-30T16:50:08Z

Mind elaborating on this change? Also, can we get a citgm run?

silverwind · 2016-05-02T19:34:51Z

Rebased.

@benjamingr:

When we introduced require('.') it had broken a undocumented use case (#1356) where NODE_PATH was set to a module's directory (as opposed to the recommended way of setting it to a directory containing module directories).

I introduced a 'hack' to restore the functionality with a deprecation message in place (#1363), which is now removed again. When this goes into the 7.0.0 release, the deprecation will have been in place for 1.5 years.

silverwind · 2016-05-02T19:36:01Z

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

ChALkeR · 2016-05-02T20:31:09Z

Perhaps we should review the stability levels in the documentation first?
Filed #6528 for that.

silverwind · 2016-05-02T21:17:56Z

Test is broken. Looks like something in ae18bbe made my "maintain backwards compat" section neccessary to resolve '.' correctly, investigating.

silverwind · 2016-05-02T22:26:25Z

Condition fixed, new CI: https://ci.nodejs.org/job/node-test-pull-request/2468/

silverwind · 2016-05-10T21:11:44Z

CI is green except an unrelated java failure. I don't think a CITGM would bring any more insights here because modules usually don't set NODE_PATH.

silverwind · 2016-05-17T19:20:30Z

@thealphanerd can you run this through a CITGM, just to be sure?

MylesBorins · 2016-05-17T19:29:35Z

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/273/

ChALkeR · 2016-05-17T22:06:53Z

Note that this change currently directly contradicts with the documentation, which says that only security, performance, or bug fixes will be accepted for module.

Either the documentation should be fixed first, or this change rejected.

silverwind · 2016-06-15T19:33:16Z

lib/module.js

@mscdex does this change look right to you? The condition was start !== '.' && start !== './' && start !== '..' before you refactored it to use charCodeAt. Tests are passing, but I'd like to get a second look on that one.

You'll want to add appropriate request length checking before using charCodeAt(n) to make sure you don't cause a deopt due to an out-of-bounds access.

silverwind · 2016-06-15T19:40:54Z

While #6528 is still in limbo, I'd like to get this hack removed rather sooner than later.

@nodejs/collaborators any concerns regarding the changes? (See #3384 (comment) for a summary)

mscdex · 2016-06-15T22:00:28Z

test/parallel/test-require-dot.js

Instead of removing this test should we merely assert the opposite?

silverwind · 2016-09-02T17:52:43Z

Totally forgot about this one. I'll move it to the next milestone.

BridgeAR · 2017-08-24T23:01:42Z

+1 for option 2

silverwind · 2017-08-25T17:10:14Z

I don't assume that a CITGM will catch anything here. The compat is just for users that set NODE_PATH manually, which I assume no sane module would do by itself. Oh, and those users now have been seeing the deprecation for almost 2.5 years. I'll start a CI run now.

silverwind · 2017-08-30T15:06:44Z

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

(For real now, couldn't start the last time because the CI was down)

jasnell · 2017-08-30T17:07:40Z

@nodejs/ctc ... please take a look at this and weigh in.

~~@BridgeAR~~ (I totally meant to mention @silverwind rather than @BridgeAR here) ... this would also need to update the documentation in docs/api/deprecations.md to mark that the deprecation is at the End-of-Life stage.

silverwind · 2017-08-30T18:09:53Z

@jasnell done.

I'll land this tomorrow if there are no objections.

jdalton · 2017-08-30T23:16:43Z

🎉 which Node version is this expected to land in?

jasnell · 2017-08-30T23:18:29Z

It's a major so it would land in 9.0.0

silverwind · 2017-08-31T19:17:18Z

Finally landed in a517466. It's been a long ride.

This removes the compatibilty code that was in place to allow an unintended interaction between `require('.')` and `NODE_PATH`. The compatibility code and the accompanying deprecation warning has been in place since 2015-04-17. PR-URL: #3384 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This removes the compatibilty code that was in place to allow an unintended interaction between `require('.')` and `NODE_PATH`. The compatibility code and the accompanying deprecation warning has been in place since 2015-04-17. PR-URL: nodejs/node#3384 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2017-10-31T05:59:33Z

@nodejs/tsc it appears that this landed without running citgm and it is breaking a fairly insignificant portion of our ecosystem.

It breaks a common use case of the module d specifically:

require('d/lazy')

A revert is incoming. If we had caught this earlier we may have been able to come up with a fix but with d receiving over 500k downloads a day I don't see how we can ship this tomorrow

The original commit was landed without running CITGM. Unfortunately this change breaks the module `d` which has over 500k downloads a day. It is worth mentioning that the compatibility hack can be removed without breaking anything. We should definitely revisit for the next Semver-Major but shipping this today will cause non trivial ecosystem breakages. Refs: nodejs#3384

MylesBorins · 2017-10-31T06:36:45Z

Some more details after a bit more research

this commit specifically broke the case of requiring individual files in modules that only have a single character as a name. This will thankfully be limited to 26 cases!

The specific line that broke things is a517466#diff-d1234a869b3d648ebfcdce5a76747d71L338

In light of this specific edge case we may want to re-examine the deprecation all together

The original commit was landed without running CITGM. Unfortunately this change breaks the module `d` which has over 500k downloads a day. It is worth mentioning that the compatibility hack can be removed without breaking anything. We should definitely revisit for the next Semver-Major but shipping this today will cause non trivial ecosystem breakages. Refs: #3384 PR-URL: #16634 Refs: #3384 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

The original commit was landed without running CITGM. Unfortunately this change breaks the module `d` which has over 500k downloads a day. It is worth mentioning that the compatibility hack can be removed without breaking anything. We should definitely revisit for the next Semver-Major but shipping this today will cause non trivial ecosystem breakages. Refs: nodejs/node#3384 PR-URL: nodejs/node#16634 Refs: nodejs/node#3384 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

In certain cases, `require('.')` could resolve outside the package directory. This behavior has been removed. PR-URL: nodejs#26973 Refs: nodejs#3384 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

silverwind added module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 15, 2015

silverwind added this to the 6.0.0 milestone Oct 15, 2015

silverwind mentioned this pull request Oct 15, 2015
module: remove require('.') with NODE_PATH compatibilty #1452
Closed

silverwind removed this from the 6.0.0 milestone Dec 1, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8dCompare December 27, 2015 02:00

silverwind added this to the 7.0.0 milestone Feb 15, 2016

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompare April 26, 2016 05:22

silverwind force-pushed the module-remove-dot-hack branch from 06275b9 to 5cb631fCompare May 2, 2016 19:29

ChALkeR mentioned this pull request May 2, 2016
API stability level «Locked» is inaccurate #6528
Closed

silverwind reviewed Jun 15, 2016
View reviewed changes

mscdex reviewed Jun 15, 2016
View reviewed changes

silverwind added this to the 8.0.0 milestone Sep 2, 2016

BridgeAR approved these changes Aug 25, 2017
View reviewed changes

jasnell approved these changes Aug 30, 2017
View reviewed changes

doc: mark DEP0019 as End-of-Life
c72dbea

targos approved these changes Aug 30, 2017
View reviewed changes

thefourtheye approved these changes Aug 30, 2017
View reviewed changes

silverwind closed this Aug 31, 2017

silverwind deleted the module-remove-dot-hack branch August 31, 2017 19:17

Trott removed the ctc-review label Sep 2, 2017

jasnell mentioned this pull request Oct 30, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
Merged

MylesBorins mentioned this pull request Oct 31, 2017
module: revert #3384 DEP0019 EOL #16634
Closed

BridgeAR mentioned this pull request Mar 29, 2019
module: mark DEP0019 as End-of-Life #26973
Closed
4 tasks

Uh oh!

module: remove require('.') with NODE_PATH compat#3384

module: remove require('.') with NODE_PATH compat #3384

Uh oh!

Conversation

silverwind commented Oct 15, 2015

Uh oh!

silverwind commented Oct 15, 2015

Uh oh!

silverwind commented Dec 1, 2015

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

silverwind commented Mar 22, 2016

Uh oh!

benjamingr commented Apr 30, 2016

Uh oh!

silverwind commented May 2, 2016

Uh oh!

silverwind commented May 2, 2016

Uh oh!

ChALkeR commented May 2, 2016

Uh oh!

silverwind commented May 2, 2016

Uh oh!

silverwind commented May 2, 2016

Uh oh!

silverwind commented May 10, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented May 17, 2016

Uh oh!

MylesBorins commented May 17, 2016

Uh oh!

ChALkeR commented May 17, 2016

Uh oh!

silverwindJun 15, 2016

Choose a reason for hiding this comment

Uh oh!

mscdexJun 15, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jun 15, 2016

Uh oh!

mscdexJun 15, 2016

Choose a reason for hiding this comment

Uh oh!

silverwindJun 15, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind commented Sep 2, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Aug 24, 2017

Uh oh!

silverwind commented Aug 25, 2017

Uh oh!

silverwind commented Aug 30, 2017

Uh oh!

jasnell commented Aug 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 30, 2017

Uh oh!

jdalton commented Aug 30, 2017

Uh oh!

jasnell commented Aug 30, 2017

Uh oh!

silverwind commented Aug 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

silverwind commented May 10, 2016•
edited
Loading

mscdexJun 15, 2016•
edited
Loading

silverwind commented Sep 2, 2016•
edited
Loading

jasnell commented Aug 30, 2017•
edited
Loading