module: support multi-dot file extensions #23416

GeoffreyBooth · 2018-10-11T00:34:14Z

Following up from #23228 and nodejs/citgm#604.

This PR adds support for registering “multi-dot” file extensions (like .coffee.md) to the module loader. Previously userland modules like CoffeeScript were patching Module.prototype.load to support code like require.extensions['.foo.bar'] = .... After this PR, such patching is no longer necessary. This also resolves the original issue that the reverted #23228 was addressing, fixing inconsistency between load and _findPath.

Written with @jdalton.

Checklist

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

addaleax · 2018-10-11T05:15:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17738/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/243/ (no significant changes)

mscdex · 2018-10-11T07:16:01Z

lib/internal/modules/cjs/loader.js

+    startIndex = index + 1;
+    if (index === 0) continue; // Skip dotfiles like .gitignore
+    currentExtension = name.slice(index);
+    if (Module._extensions[currentExtension]) return currentExtension;


Couldn't we just store and return the function instead to avoid a second, unnecessary lookup?

@mscdex Would you clarify your question (maybe with pseudo code)?

The value of Module._extensions[currentExtension] is thrown away here. However after this function returns, Module._extensions[extension] is done a second time. It would be nice if we could avoid this duplicate lookup.

So instead of:

currentExtension = name.slice(index); if (Module._extensions[currentExtension]) return currentExtension;

we could do:

const loader = Module._extensions[name.slice(index)]; if (loader) return loader;

and change the calling code appropriately:

findExtension(filename)(this, filename);

The function name and the default return value would need to be changed of course.

I wrote some code to do a performance test:

const path = require('path'); // Mock Module for testing const Module = { _extensions: { '.js': function () { const makeWork = JSON.parse('{"foo": "bar", "baz": 123}'); return makeWork; }, '.json': function () { const makeWork = JSON.parse('{"foo": "bar", "baz": 123}'); return makeWork; }, '.node': function () { const makeWork = JSON.parse('{"foo": "bar", "baz": 123}'); return makeWork; } } } // Generate 100000 filenames const filenames = []; for (let i = 0; i < 100000; ++i) { filenames.push(`${i}.${Module._extensions[i % 3]}`); } // Current version function findExtension(filename) { const name = path.basename(filename); let currentExtension; let index; let startIndex = 0; while ((index = name.indexOf('.', startIndex)) !== -1) { startIndex = index + 1; if (index === 0) continue; // Skip dotfiles like .gitignore currentExtension = name.slice(index); if (Module._extensions[currentExtension]) return currentExtension; } return '.js'; } // Altered version that returns the loader itself rather than the extension, // so that there’s only one lookup function findLoaderByExtension(filename) { const name = path.basename(filename); let currentExtension; let index; let startIndex = 0; let loader; while ((index = name.indexOf('.', startIndex)) !== -1) { startIndex = index + 1; if (index === 0) continue; // Skip dotfiles like .gitignore currentExtension = name.slice(index); if (loader = Module._extensions[currentExtension]) return loader; } return Module._extensions['.js']; } const run = { test1: function() { console.time('test1'); filenames.forEach((filename) => { var extension = findExtension(filename); Module._extensions[extension](this, filename); }); console.timeEnd('test1'); }, test2: function() { console.time('test2'); filenames.forEach((filename) => { var loader = findLoaderByExtension(filename); loader(this, filename); }); console.timeEnd('test2'); } } run[process.argv[2]]();

Here are my results:

✦ node -v v10.12.0 ✦ node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 && node test.js test1 test1: 85.108ms test1: 85.046ms test1: 87.216ms test1: 85.767ms test1: 85.926ms test1: 85.650ms test1: 84.461ms test1: 85.011ms test1: 83.525ms test1: 87.337ms ✦ node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 && node test.js test2 test2: 90.877ms test2: 86.902ms test2: 86.268ms test2: 85.143ms test2: 90.064ms test2: 86.636ms test2: 84.882ms test2: 86.778ms test2: 89.278ms test2: 85.963ms

Test 1 average time: 85.505ms

Test 2 average time: 87.279ms

@GeoffreyBooth My suggestion was not so much about a huge performance improvement as it is removing duplicated effort.

Also, if you really want to do benchmarks, they should be done via the official benchmark mechanism for the best comparison.

The first version has two lookups, but the second version has an extra assignment (loader = Module._extensions[currentExtension]). So the “return the loader” version saves a second lookup at the cost of an extra assignment, which I think explains why the benchmarks are more or less equivalent between the two versions. Personally I think a helper that returns an extension is easier to grasp (and more potentially useful in the future) than a helper that returns a loader, but that’s a subjective call.

thefourtheye · 2018-10-11T12:53:25Z

lib/internal/modules/cjs/loader.js

+  let startIndex = 0;
+  while ((index = name.indexOf('.', startIndex)) !== -1) {
+    startIndex = index + 1;
+    if (index === 0) continue; // Skip dotfiles like .gitignore


If we are to skip the dotfiles, shouldn't this just break out of here?

@thefourtheye

If we are to skip the dotfiles, shouldn't this just break out of here?

No, because it's not skipping the whole dotfile, just the first dot of the dot file.

path.extname(".dotfile") // => "" path.extname(".dotfile.ext") // => ".ext"

A test was added for this case.

bnoordhuis · 2018-10-11T21:28:24Z

lib/internal/modules/cjs/loader.js

+// find the longest (possibly multi-dot) extension registered in
+// Module._extensions
+function findExtension(filename) {
+  const name = path.basename(filename);


If you're only interested in the extension, is calling path.basename() necessary? You could just scan backwards for dots, checking that there are no path separators in between:

let dot = -1; let sep = -1; for (let i = filename.length; --i > 0; /* empty */) { const c = filename[i]; if (c === '.') dot = i; else if (c === '/') { // FIXME handle backslash on windows sep = i; break; } } if (dot > sep + 1) { const ext = filename.slice(dot); if (Module._extensions[ext]) return ext; } return '.js'; // dot file or no extension

Only one slice == less garbage to collect.

The current algorithm scans forwards to short-circuit at the longest matched extension. So say you have a file like mr.robot.coffee.md, and a loader registered for .coffee.md. The current algorithm will iterate like:

Is .robot.coffee.md registered? No, continue.

Is .coffee.md registered? Yes, break.

This way, even if .md is also registered, the .coffee.md loader takes precedence. This allows multi-dot extensions like .es6.js, for example.

If I’m reading your code correctly, it’s checking only the longest possible multi-dot extension, which for this example would be .robot.coffee.md, a false match. We want multi-dot extensions to work without prohibiting periods elsewhere in the filename.

Perhaps naming the function findRegisteredExtension would be more self-descriptive?

@richardlau Yes, or perhaps findLongestRegisteredExtension would be even more descriptive (because we’re returning .coffee.md instead of .md, if both of those happened to be registered).

GeoffreyBooth · 2018-10-15T04:48:56Z

@addaleax I rewrote the commit messages to get the build to pass. Does this PR need anything else?

jdalton · 2018-10-25T17:11:17Z

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

jdalton · 2018-10-25T17:46:15Z

I believe this is author ready. Please feel free to remove the label for any reason.

addaleax · 2018-10-25T18:26:35Z

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

Trott · 2018-11-02T02:28:47Z

Landed in 22f7d0a

Support multi-dot file extensions like '.coffee.md' in Module.load. PR-URL: nodejs#23416 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]>

Support multi-dot file extensions like '.coffee.md' in Module.load. PR-URL: #23416 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]>

addaleax approved these changes Oct 11, 2018

View reviewed changes

GeoffreyBooth force-pushed the multi-ext branch from 1fb0f8b to 9f4d00d Compare October 11, 2018 06:12

mscdex reviewed Oct 11, 2018

View reviewed changes

thefourtheye reviewed Oct 11, 2018

View reviewed changes

bnoordhuis reviewed Oct 11, 2018

View reviewed changes

GeoffreyBooth force-pushed the multi-ext branch 3 times, most recently from efbf50a to 6908da4 Compare October 15, 2018 04:29

GeoffreyBooth added 4 commits October 23, 2018 16:07

module: support multi-dot file extension ('.coffee.md') in Module.load

a0a401d

module: tests for multi-dot extensions

47ca0eb

module: rename function to be more descriptive

06f1afd

module: dotfile tests

64aab09

GeoffreyBooth force-pushed the multi-ext branch from 6908da4 to 64aab09 Compare October 23, 2018 23:30

jdalton approved these changes Oct 25, 2018

View reviewed changes

jasnell added module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 25, 2018

jdalton added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 25, 2018

Trott closed this Nov 2, 2018

GeoffreyBooth deleted the multi-ext branch November 2, 2018 03:58

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

This was referenced Nov 15, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#232

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-android-nodejs#249

Closed

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: support multi-dot file extensions #23416

module: support multi-dot file extensions #23416

GeoffreyBooth commented Oct 11, 2018 •

edited

Loading

addaleax commented Oct 11, 2018 •

edited

Loading

mscdex Oct 11, 2018

jdalton Oct 11, 2018

mscdex Oct 11, 2018 •

edited

Loading

GeoffreyBooth Oct 12, 2018

mscdex Oct 12, 2018

GeoffreyBooth Oct 12, 2018

thefourtheye Oct 11, 2018

jdalton Oct 11, 2018

GeoffreyBooth Oct 25, 2018

bnoordhuis Oct 11, 2018

GeoffreyBooth Oct 12, 2018 •

edited

Loading

richardlau Oct 12, 2018

GeoffreyBooth Oct 12, 2018

GeoffreyBooth commented Oct 15, 2018

jdalton commented Oct 25, 2018

jdalton commented Oct 25, 2018

addaleax commented Oct 25, 2018

Trott commented Nov 2, 2018

module: support multi-dot file extensions #23416

module: support multi-dot file extensions #23416

Conversation

GeoffreyBooth commented Oct 11, 2018 • edited Loading

Checklist

addaleax commented Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Oct 15, 2018

jdalton commented Oct 25, 2018

jdalton commented Oct 25, 2018

addaleax commented Oct 25, 2018

Trott commented Nov 2, 2018

GeoffreyBooth commented Oct 11, 2018 •

edited

Loading

addaleax commented Oct 11, 2018 •

edited

Loading

mscdex Oct 11, 2018 •

edited

Loading

GeoffreyBooth Oct 12, 2018 •

edited

Loading