module: fix inconsistency between load and _findPath #22382

lundibundi · 2018-08-17T23:31:34Z

Files with multiple extensions are not handled by require-module system
therefore if you have file 'file.foo.bar' and require('./file') it won't
be found even while using require.extensions['foo.bar'] but before this
commit if you have require.extensions['foo.bar'] and
require.extensions['bar'] set then the latter will be called if you do
require('./file') but if you remove the former ('foo.bar')
property it will fail.
This commit makes it always fail in such cases.

Fixes: #4778

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

mscdex · 2018-08-18T00:57:55Z

lib/internal/modules/cjs/loader.js

@@ -216,6 +216,11 @@ function tryExtensions(p, exts, isMain) {
  return false;
 }

+function readExtensions() {
+  return Object.keys(Module._extensions)
+    .map((ext) => path.extname(ext) || ext);


Can we avoid the functional array methods and use a regular loop instead?

I'm not against this but imo this looks much better than regular loop would. As for the performance implications this is not a hot code and also in the newest v8 AFAIK functional operations have the same or at least similar performance to regular loops.
Though if you want I'll change it. I think it will be as follows.

const exts = Object.keys(Module._extensions); const result = new Array(exts.length); for (let i = 0; i < exts.length; ++i) result[i] = path.extname(exts[i]) || exts[i]; return result;

I think you could simplify if further and make it even more efficient than a straight conversion with:

const exts = Object.keys(Module._extensions); for (let i = 0; i < exts.length; ++i) exts[i] = path.extname(exts[i]) || exts[i]; return exts;

or avoid reassigning the same value:

const exts = Object.keys(Module._extensions); for (let i = 0; i < exts.length; ++i) { const extName = path.extname(exts[i]); if (extName) exts[i] = extName; } return exts;

Oh, yeah. Functional thinking got too far =). Though I think the first method you suggested should be fine already.

BridgeAR

LGTM

jdalton · 2018-08-21T23:06:36Z

lib/internal/modules/cjs/loader.js

+  for (var i = 0; i < exts.length; ++i)
+    exts[i] = path.extname(exts[i]) || exts[i];
+  return exts;
+}


In the example case of require.extensions of .foo.bar and .bar this will return an array of ['.bar', '.bar]?

So if you delete require.extensions[".bar"] files like test-extensions.foo.bar will still be found but their compiler at require.extensions[".foo.bar"] won't be used for the module since _compile will use extname(filename) to and get .bar which doesn't exist on require.extensions and so will default to .js.

Good catch, though I'm not sure what's the correct behavior here. I'd say to just filter-out all extensions that path.extname() !== '' and use the remaining ones, as they shouldn't be used anyway, wdyt?

jdalton

This patch makes additions like require.extensions[".foo.bar"] findable but not runnable as it defaults to require.extensions[".js"] since the extname(filename) is ".bar" and does not have an entry in require.extensions. This means this PR isn't really finished. Finding a module is great but it needs to use the correct compiler too.

Files with multiple extensions are not handled by require-module system therefore if you have file 'file.foo.bar' and require('./file') it won't be found even while using require.extensions['foo.bar'] but before this commit if you have require.extensions['foo.bar'] and require.extensions['bar'] set then the latter will be called if you do require('./file') but if you remove the former the former ('foo.bar') property it will fail. This commit makes it always fail in such cases. Fixes: nodejs#4778

lundibundi · 2018-08-23T00:13:55Z

@jdalton PTAL #22382 (comment), I've changed the implementation to ignore multiple-extensions-type (as this is from what I understand compiler does via https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L597). The idea of this PR is to forbid what was previously accepted but is incorrect (if you have require.extensions[".foo.bar"] = 'anything' and require.extensions[".bar"] = () => {} then the latter was called upon require('./file') assuming we had file.foo.bar file). It doesn't make them findable, it does the contrary and makes the not findable.

I've also added the test for the use-case you've described (have '.foo.bar' in extensions and call require('./test-extensions.foo') which should fail) https://github.com/nodejs/node/pull/22382/files#diff-a3f5781fd9530e809df569c7da7cd2a7R33.

addaleax · 2018-08-27T21:28:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16797/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/229/

lundibundi · 2018-08-29T00:17:44Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/16842/

addaleax · 2018-09-02T11:26:56Z

@lundibundi Do you want to land this? It seems ready to me. :)

lundibundi · 2018-09-02T13:52:06Z

@addaleax thanks for reminding I kinda forgot =)
Wanted to make CI happy finally.
Another try: https://ci.nodejs.org/job/node-test-pull-request/16947/

addaleax · 2018-09-02T16:16:30Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16959/ (:heavy_check_mark:)

lundibundi · 2018-09-02T19:42:28Z

~~Another resume: https://ci.nodejs.org/job/node-test-pull-request/16973/~~
tough fight 🙃.

On and on: https://ci.nodejs.org/job/node-test-pull-request/16979/ (:heavy_check_mark:)

addaleax · 2018-09-03T12:47:25Z

@lundibundi I’m not sure I understand – all of the last three CIs (16959, 16973, 16979) were good…?

lundibundi · 2018-09-03T14:30:07Z

Oh, I kind of didn't check that the failed test was with node help options. 🙃

Though I wanted to land this already, but node-core-utils (git-node) for some reason gets stuck on network operations and cannot reset my master with upstream/master...
Will try again, just skip that step I guess, I can always manually rebase on upstream/master.
Oh well, now core-validate-commit crashes. I guess I'm gonna reinstall both of them as well as nvm env.

EDIT: Okay, I guess the issues are not going anywhere, I kind of don't want to land when my npm/node/git is in such state. I'll try to debug them.
For the time being, @addaleax could you please land this? I don't think this should wait any longer.

addaleax

LGTM but I’m adding semver-major since it sounds like there’s a small chance of breakage, feel free to remove the label if I’m mistaken

addaleax · 2018-09-17T15:42:33Z

Landed in 1b92214

Files with multiple extensions are not handled by require-module system therefore if you have file 'file.foo.bar' and require('./file') it won't be found even while using require.extensions['foo.bar'] but before this commit if you have require.extensions['foo.bar'] and require.extensions['bar'] set then the latter will be called if you do require('./file') but if you remove the former the former ('foo.bar') property it will fail. This commit makes it always fail in such cases. Fixes: #4778 PR-URL: #22382 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This reverts commit 1b92214 from PR #22382. See the discussion at nodejs/citgm#604 PR-URL: #23228 Refs: #22382 Fixes: #4778 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

mscdex reviewed Aug 18, 2018

View reviewed changes

lundibundi force-pushed the fix-module-load-inconsistency branch from 6dd03d6 to 9a0ad77 Compare August 18, 2018 22:26

BridgeAR approved these changes Aug 21, 2018

View reviewed changes

jdalton reviewed Aug 21, 2018

View reviewed changes

lundibundi force-pushed the fix-module-load-inconsistency branch from 9a0ad77 to 991cacf Compare August 22, 2018 11:19

jdalton suggested changes Aug 22, 2018

View reviewed changes

lundibundi added 3 commits August 23, 2018 02:58

fixup! module: fix inconsistency between load and _findPath

1b51f7e

fixup! module: fix inconsistency between load and _findPath

b009708

lundibundi force-pushed the fix-module-load-inconsistency branch from 991cacf to b009708 Compare August 23, 2018 00:04

jdalton approved these changes Aug 23, 2018

View reviewed changes

lundibundi added module Issues and PRs related to the module subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 23, 2018

jasnell approved these changes Sep 4, 2018

View reviewed changes

addaleax approved these changes Sep 17, 2018

View reviewed changes

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 17, 2018

addaleax closed this Sep 17, 2018

targos mentioned this pull request Sep 23, 2018

coffeescript is broken on current master nodejs/citgm#604

Closed

jdalton mentioned this pull request Oct 2, 2018

Revert "module: fix inconsistency between load and _findPath" #23228

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: fix inconsistency between load and _findPath #22382

module: fix inconsistency between load and _findPath #22382

lundibundi commented Aug 17, 2018 •

edited

Loading

mscdex Aug 18, 2018

lundibundi Aug 18, 2018

mscdex Aug 18, 2018

lundibundi Aug 18, 2018 •

edited

Loading

BridgeAR left a comment

jdalton Aug 21, 2018 •

edited

Loading

lundibundi Aug 22, 2018

jdalton left a comment •

edited

Loading

lundibundi commented Aug 23, 2018 •

edited

Loading

addaleax commented Aug 27, 2018

lundibundi commented Aug 29, 2018

addaleax commented Sep 2, 2018

lundibundi commented Sep 2, 2018

addaleax commented Sep 2, 2018 •

edited

Loading

lundibundi commented Sep 2, 2018 •

edited by addaleax

Loading

addaleax commented Sep 3, 2018

lundibundi commented Sep 3, 2018 •

edited

Loading

addaleax left a comment

addaleax commented Sep 17, 2018

module: fix inconsistency between load and _findPath #22382

module: fix inconsistency between load and _findPath #22382

Conversation

lundibundi commented Aug 17, 2018 • edited Loading

Checklist

mscdex Aug 18, 2018

Choose a reason for hiding this comment

lundibundi Aug 18, 2018

Choose a reason for hiding this comment

mscdex Aug 18, 2018

Choose a reason for hiding this comment

lundibundi Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

jdalton Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

lundibundi Aug 22, 2018

Choose a reason for hiding this comment

jdalton left a comment • edited Loading

Choose a reason for hiding this comment

lundibundi commented Aug 23, 2018 • edited Loading

addaleax commented Aug 27, 2018

lundibundi commented Aug 29, 2018

addaleax commented Sep 2, 2018

lundibundi commented Sep 2, 2018

addaleax commented Sep 2, 2018 • edited Loading

lundibundi commented Sep 2, 2018 • edited by addaleax Loading

addaleax commented Sep 3, 2018

lundibundi commented Sep 3, 2018 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Sep 17, 2018

lundibundi commented Aug 17, 2018 •

edited

Loading

lundibundi Aug 18, 2018 •

edited

Loading

jdalton Aug 21, 2018 •

edited

Loading

jdalton left a comment •

edited

Loading

lundibundi commented Aug 23, 2018 •

edited

Loading

addaleax commented Sep 2, 2018 •

edited

Loading

lundibundi commented Sep 2, 2018 •

edited by addaleax

Loading

lundibundi commented Sep 3, 2018 •

edited

Loading