module: revert #3384 DEP0019 EOL by MylesBorins · Pull Request #16634 · nodejs/node · GitHub

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

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

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

can you post the citgm failure here please?

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.

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

@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

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

@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

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

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/

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

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

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

Both of which appear to be flaky

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

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>

Landed in 45873d2

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))) {

@jdalton do you want to take a pass at a fix + test in a new PR?

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.

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>

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 31, 2017

MylesBorins requested a review from jasnell October 31, 2017 06:47

jasnell approved these changes Oct 31, 2017

View reviewed changes

MylesBorins mentioned this pull request Oct 31, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

apapirovski approved these changes Oct 31, 2017

View reviewed changes

cjihrig approved these changes Oct 31, 2017

View reviewed changes

jasnell closed this Oct 31, 2017

jdalton reviewed Oct 31, 2017

View reviewed changes

MylesBorins added the dont-land-on-v4.x label Nov 14, 2017

MylesBorins added dont-land-on-v6.x labels Nov 14, 2017

MylesBorins deleted the revert-3384 branch November 14, 2017 17:44

+                    request.charCodeAt(0) !== 46/*.*/ ||
                     (request.charCodeAt(1) !== 46/*.*/ &&
                      request.charCodeAt(1) !== 47/*/*/)) {
                   var paths = modulePaths;

module: revert #3384 DEP0019 EOL by MylesBorins · Pull Request #16634 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: revert #3384 DEP0019 EOL - #16634

module: revert #3384 DEP0019 EOL#16634MylesBorins wants to merge 1 commit intonodejs:masternodejs/node:masterfrom MylesBorins:revert-3384MylesBorins/node:revert-3384Copy head branch name to clipboard

Conversation

MylesBorins commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

targos commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

targos commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jdalton commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

MylesBorins commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jasnell commented Oct 31, 2017

Uh oh!

jdalton Oct 31, 2017 • 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!

MylesBorins commented Oct 31, 2017 via email

Uh oh!

jdalton commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

module: revert #3384 DEP0019 EOL#16634
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:revert-3384

jasnell commented Oct 31, 2017 •
edited
Loading

jdalton commented Oct 31, 2017 •
edited
Loading

jasnell commented Oct 31, 2017 •
edited
Loading

MylesBorins commented Oct 31, 2017 •
edited
Loading

jdalton Oct 31, 2017 •
edited
Loading

jdalton commented Oct 31, 2017 •
edited
Loading