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.

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

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

Closed
wants to merge 2 commits into from

Conversation

silverwind
Copy link
Contributor

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

@silverwind 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 silverwind added this to the 6.0.0 milestone Oct 15, 2015
@silverwind
Copy link
Contributor Author

@silverwind silverwind removed this from the 6.0.0 milestone Dec 1, 2015
@silverwind
Copy link
Contributor Author

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

jasnell commented Mar 22, 2016

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

@silverwind
Copy link
Contributor Author

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

@benjamingr
Copy link
Member

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

@silverwind
Copy link
Contributor Author

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

@ChALkeR
Copy link
Member

ChALkeR commented May 2, 2016

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

@silverwind
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor Author

silverwind commented May 10, 2016

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

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

@MylesBorins
Copy link
Contributor

@ChALkeR
Copy link
Member

ChALkeR commented May 17, 2016

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.

lib/module.js Outdated
if (request.charCodeAt(0) !== 46/*.*/ || (
request.charCodeAt(1) !== 46/*.*/ &&
request.charCodeAt(1) !== 47/*/*/ &&
request.length > 1)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@mscdex mscdex Jun 15, 2016

Choose a reason for hiding this comment

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

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

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)


var c = require('.');

assert.equal(c.value, 42, 'require(".") should honor NODE_PATH');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@silverwind
Copy link
Contributor Author

silverwind commented Sep 2, 2016

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

@silverwind silverwind added this to the 8.0.0 milestone Sep 2, 2016
@BridgeAR
Copy link
Member

+1 for option 2

@silverwind
Copy link
Contributor Author

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

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

jasnell commented Aug 30, 2017

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

@jasnell done.

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

@jdalton
Copy link
Member

jdalton commented Aug 30, 2017

🎉 which Node version is this expected to land in?

@jasnell
Copy link
Member

jasnell commented Aug 30, 2017

It's a major so it would land in 9.0.0

@silverwind
Copy link
Contributor Author

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

@silverwind silverwind closed this Aug 31, 2017
@silverwind silverwind deleted the module-remove-dot-hack branch August 31, 2017 19:17
silverwind added a commit that referenced this pull request Aug 31, 2017
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]>
@Trott Trott removed the ctc-review label Sep 2, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
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
Copy link
Contributor

@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

MylesBorins added a commit to MylesBorins/node that referenced this pull request Oct 31, 2017
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
Copy link
Contributor

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

jasnell pushed a commit that referenced this pull request Oct 31, 2017
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]>
jasnell pushed a commit that referenced this pull request Oct 31, 2017
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]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
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]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 3, 2019
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.