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: revert #3384 DEP0019 EOL #16634

Closed
wants to merge 1 commit into from
Closed

module: revert #3384 DEP0019 EOL #16634

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

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

@MylesBorins
module: revert nodejs#3384 DEP0019 EOL
6bad984 
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
@nodejs-github-bot nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 31, 2017
@MylesBorins
Copy link
Contributor Author

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

@targos
Copy link
Member

targos commented Oct 31, 2017

can you post the citgm failure here please?

@MylesBorins
Copy link
Contributor Author

here is an example of one of the failures

module.js:520
    throw err;
    ^

Error: Cannot find module 'd/lazy'
    at Function.Module._resolveFilename (module.js:518:15)
    at Function.Module._load (module.js:448:25)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/memoizee/lib/methods.js:6:21)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/memoizee/methods.js:3:18)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/cli-color/bare.js:10:22)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/cli-color/index.js:5:42)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/tad/lib/console.js:13:17)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/tad/index.js:8:20)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Module.require (module.js:561:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/mborins/code/d/node_modules/tad/bin/tad:22:13)
    at Module._compile (module.js:617:30)
    at Object.Module._extensions..js (module.js:628:10)
    at Module.load (module.js:536:32)
    at tryModuleLoad (module.js:479:12)
    at Function.Module._load (module.js:471:3)
    at Function.Module.runMain (module.js:658:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:613:3
npm ERR! Test failed.  See above for more details.

@targos
Copy link
Member

targos commented Oct 31, 2017

thanks. I'm surprised we don't test this use of require. It is common to do that with lodash.

@MylesBorins
Copy link
Contributor Author

@targos it seems like this commit specifically broke the case of requiring specific files in modules that only have a single character as a name. This will thankfully be limited to 26 cases! Hopefully this hint can help us with maybe landing this in the future

@MylesBorins MylesBorins requested a review from jasnell October 31, 2017 06:47
@MylesBorins MylesBorins mentioned this pull request Oct 31, 2017
Merged
@jasnell
Copy link
Member

jasnell commented Oct 31, 2017 •
edited
Loading

CI failed completely on windows. Trying again https://ci.nodejs.org/job/node-test-commit-windows-fanned/13041/

@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

@MylesBorins ...

it seems like this commit specifically broke the case of requiring specific files in modules that only have a single character as a name.

A regression test for specifically this case would be helpful as a separate PR

@jdalton
Copy link
Member

jdalton commented Oct 31, 2017 •
edited
Loading

I don't think my implementation has the regression. I can import lazy from "d/lazy" at least.
(might be useful as a reference when fixing later)

Ah, nervermind. When forcing it on (was some semver -rc detection wonkiness) I get errors too 😢

Error [ERR_MISSING_MODULE]: Cannot find module d/lazy

@jasnell
Copy link
Member

jasnell commented Oct 31, 2017 •
edited
Loading

Trying CI on Windows again now that #16639 is landed: https://ci.nodejs.org/job/node-test-commit-windows-fanned/13045/ ... that failed, but I may have restarted it wrong... trying again https://ci.nodejs.org/job/node-test-pull-request/11117/

@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

got past the compile error on windows... should be good but letting the CI finish

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Oct 31, 2017 •
edited
Loading

single windows failures parallel/test-async-wrap-uncaughtexception

edit: make that two sequential/test-inspector-async-call-stack

@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

Both of which appear to be flaky

@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

Just waiting on AIX and OSX to finish... then will get this landed

jasnell pushed a commit that referenced this pull request Oct 31, 2017
@MylesBorins @jasnell
module: revert #3384 DEP0019 EOL
45873d2 
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 <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 31, 2017
@MylesBorins @jasnell
module: revert #3384 DEP0019 EOL
82790d8 
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 <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 31, 2017

Landed in 45873d2

@jasnell jasnell closed this Oct 31, 2017
jdalton
jdalton reviewed Oct 31, 2017
View reviewed changes
lib/module.js
@@ -334,7 +347,8 @@ Module._resolveLookupPaths = function(request, parent, newReturn) {
}

// Check for relative path
if (request.charCodeAt(0) !== 46/*.*/ &&
if (request.length < 2 ||
request.charCodeAt(0) !== 46/*.*/ ||
(request.charCodeAt(1) !== 46/*.*/ &&
request.charCodeAt(1) !== 47/*/*/)) {
var paths = modulePaths;
Copy link
Member

@jdalton jdalton Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The use of charCodeAt jazz really complicates the logic. It was introduced in a pass to optimize lots of things but I suspect it was micro-opts carried a bit too far. Anyways, would this do to fix it:

if (! (request.length === 1 && 
       request.charCodeAt(0) === codeOfDot) &&
    ! (request.charCodeAt(0) === codeOfDot &&
        (request.charCodeAt(1) === codeOfDot || 
         request.charCodeAt(1) === codeOfSlash))) {

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Oct 31, 2017 via email

@jdalton
Copy link
Member

jdalton commented Oct 31, 2017 •
edited
Loading

Thanks @MylesBorins! I'm a bit time crunched at the moment but I'd dig picking it up at the end of the week if it's still available.

I'll update my comment with findings from my local implementation today.

Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@MylesBorins @Qard
module: revert #3384 DEP0019 EOL
eab836f 
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 <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@MylesBorins @Qard
module: revert #3384 DEP0019 EOL
6d7b863 
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 <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins deleted the revert-3384 branch November 14, 2017 17:44
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@MylesBorins @addaleax
module: revert #3384 DEP0019 EOL
9499459 
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 <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdalton jdalton jdalton left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MylesBorins @targos @jasnell @jdalton @apapirovski @cjihrig @nodejs-github-bot