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

Inconsistent multiple extension handling between Module._findPath and Module.load #4778

Closed
sgentle opened this issue Jan 20, 2016 · 4 comments
Labels
confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. module Issues and PRs related to the module subsystem.

Comments

@sgentle
Copy link

sgentle commented Jan 20, 2016

Let's start by making a file with multiple extensions: test.foo.bar

> fs.writeFileSync('test.foo.bar', 'NOT_JS', 'utf8')
undefined

So we already know that require.extensions doesn't work with multiple extensions:

> require.extensions['.foo.bar'] = (module, path) => console.log('required .foo.bar', path)
[Function]
> require('./test.foo.bar')
ReferenceError: NOT_JS is not defined
[...stack trace elided...]

And that it will use the last component of the extension instead:

> require.extensions['.bar'] = (module, path) => console.log('required .bar', path)
[Function]
> require('./test.foo.bar')
required .bar /private/tmp/test.foo.bar
{}

Which is all perfectly obvious and by design. But did you know that the path searching uses the whole extension? Which means:

> delete require.extensions['.foo.bar'] // Since it's never called anyway
> require('./test')
Error: Cannot find module './test'
[...stack trace elided...]

However:

> require.extensions['.foo.bar'] = "SURPRISE!"
> require('./test')
required .bar /private/tmp/test.foo.bar
{}

In summary: to hook the requiring of a file with multiple extensions without specifying it, you must:

  1. Set require.extensions for the last component of the extension to your hook function
  2. Set require.extensions for the entire extension to any value.

This is due to differences in how Module._findPath and Module.load deal with extensions: the former adds each extension in require.extensions to the basename and tests it for existence (via tryExtensions). The latter works the other way around; it removes everything up to the last part of the extension (via path.extname) and then checks if it is present in require.extensions.

I know it's policy not to fix bugs in module that might help compile-to-js languages, but I figure someone else might come across the same problem and this report could be useful to them.

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Jan 20, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jan 20, 2016

What version of Node are you using? The following script (and the variations of it that I tried) work for me on v5.4.1.

'use strict';
const assert = require('assert');
const fs = require('fs');
const file = './test.foo.bar';

fs.writeFileSync(file, 'NOT_JS', 'utf8');

require.extensions['.foo.bar'] = (module, path) => {
  console.log('required .foo.bar', path);
};

assert.throws(() => {
  require(file);
});

require.extensions['.bar'] = (module, path) => {
  console.log('required .bar', path);
};

delete require.extensions['.foo.bar']

assert.doesNotThrow(() => {
  require(file);
});

@sgentle
Copy link
Author

sgentle commented Jan 22, 2016

Sorry, to be clear, the _findPath/tryExtensions code only happens when the file is required with no extension. Try replacing your last require(file) with require('./test').

@cjihrig
Copy link
Contributor

cjihrig commented Jan 22, 2016

Ok, here is the reproduction:

'use strict';
const fs = require('fs');

fs.writeFileSync('./test.foo.bar', 'NOT_JS', 'utf8');
require.extensions['.foo.bar'] = (module, path) => {};
// delete require.extensions['.foo.bar'];
require.extensions['.bar'] = (module, path) => {
  console.log(`loaded ${path}`);
};

require('./test');

Toggle the commented out line to see the issue. This seems, to me, like something that should be fixed, but module system. What do others think?

cjihrig added a commit to cjihrig/node that referenced this issue Mar 12, 2016
This commit adds tests for several known issues.

Refs: nodejs#1901
Refs: nodejs#728
Refs: nodejs#4778
Refs: nodejs#947
Refs: nodejs#2734
PR-URL: nodejs#5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
evanlucas pushed a commit that referenced this issue Mar 14, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
rvagg pushed a commit that referenced this issue Mar 16, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Jul 7, 2017

I know it's policy not to fix bugs in module that might help compile-to-js languages, but I figure someone else might come across the same problem and this report could be useful to them.

@sgentle Fortunately, this is no longer true. And while we are unlikely to add new features or change the intended behavior of existing features, fixing bugs in module is now totally on the table. If you feel up to fixing, by all means, please submit a pull request.

@maclover7 maclover7 added the confirmed-bug Issues with confirmed bugs. label Dec 31, 2017
@apapirovski apapirovski added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 20, 2018
lundibundi added a commit to lundibundi/node that referenced this issue Aug 23, 2018
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
danbev pushed a commit that referenced this issue Oct 10, 2018
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this issue Oct 17, 2018
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants