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

Node core modules are not always prioritized over the others #56

Closed
harriha opened this issue Jul 25, 2018 · 5 comments · Fixed by #60
Closed

Node core modules are not always prioritized over the others #56

harriha opened this issue Jul 25, 2018 · 5 comments · Fixed by #60

Comments

@harriha
Copy link

harriha commented Jul 25, 2018

Hi,

Just ran into a similar issue as here but with the native querystring module.

Seems that if there's a name conflict with a node core module and a module under node_modules (or with a self-made module) and the compilerOptions.paths contains certain kind of re-mapping of all modules, tsconfig-paths does not know anymore about the core modules and starts to prefer the non-core ones.

Repro case: https://github.com/harriha/tsconfig-paths-bug . Notes:

  • npm start does things as expected
  • npm test demonstrates the bug
  • Note the paths mapping: "*": ["node_modules/*", "src/*"] - the inclusion of node_modules here is the culprit making tsconfig-paths get confused.

In practice, the repro bug is about trying to use the core querystring.escape() function, which is not present in the npm module version with same name (http://npmjs.com/package/querystring).

I guess it is highly project-specific whether the user will hit this one, but I've seen some advice for configuring the paths this way (to support custom types in custom location I presume). Having this config in my tsconfig.json is how I ended up seeing this bug in the first place, although now I realized I don't need this actually anymore and can conveniently avert this bug via removing that path mapping, but I guess there might be users out there who'll need this kind of config. Perhaps worth at least mentioning in the README somehow in case involving the step 1 from here turns out to be problematic (as I presume).

As a side-note, the TypeScript's extensive module resolution documentation doesn't seem to mention about the core modules at all, which might be a gotcha for library authors using that as the reference for the algorithm.

Edit: rename "native module" to "node core module" for clarity

@jonaskello
Copy link
Member

jonaskello commented Jul 25, 2018

By "native modules", do you mean files that are placed under node_modules/<package_name>/* ?

I don't know of a scenario where you need to put "node_modules/*" in path mappings. If you are using tsc with the node module resolution it will look there by default AFAIK. If you are executing the program with node then it will also look there by default.

Perhaps you are thinking of the scenario where you have custom typings that you write yourself (not from npm). In that case the advice has been to have a mapping like "*": ["custom_typings/*"], provided you have a folder custom_typings folder where your custom typings live.

@harriha
Copy link
Author

harriha commented Jul 25, 2018

Hi,

Thanks for the quick reply.

By "native modules", do you mean files that are placed under node_modules/<package_name>/* ?

No, I mean the Node core modules such as querystring or http and so on (apologies, my bad, confusing terminology).

I don't know of a scenario where you need to put "node_modules/*" in path mappings. If you are using tsc with the node module resolution it will look there by default AFAIK. If you are executing the program with node then it will also look there by default.

Perhaps you are thinking of the scenario where you have custom typings that you write yourself (not from npm). In that case the advice has been to have a mapping like "": ["custom_typings/"], provided you have a folder custom_typings folder where your custom typings live.

That custom typings is the case I'm thinking indeed and why I think I originally added such path mapping to my tsconfig. But as mentioned, turns out it was not needed with my current tsconfig and thus cannot give the exact scenario where that was needed, I remember fighting with getting those custom typings to work for quite some time and this path mapping probably appeared during those times. I might've found tips for doing this from somewhere in the web, but cannot locate the source right now.

So yes, all in all, in essence it was just me and my poor tsconfig in this particular case. However, considering the fact that someone else ran into similar issue with core modules and that tsconfig-paths seems to change the module resolution logic when comparing to tsc without tsconfig-paths, this might be something to address somehow regardless (via at least a README mention or so). If you feel this is an invalid issue and there's no actions to be taken, feel free to close, no worries from my side.

@harriha harriha changed the title Native modules are not always prioritized over the others Node core modules are not always prioritized over the others Jul 25, 2018
@ljani
Copy link
Contributor

ljani commented Aug 28, 2018

I actually noticed the same when I was trying to figure out why the build is suddenly so slow. At first I was looking at the wrong repository and accidentally made an issue there: dividab/tsconfig-paths-webpack-plugin#26.

The performance penalty is not acceptable IMO. Any ideas?

Here's my quick fix in tsconfig-paths/lib/register.js:

Module._resolveFilename = function (request, _parent) {
    if (request.includes("crypto")) {
        return originalResolveFilename.apply(this, arguments);
    }
    var found = matchPath(request);

@Jontem
Copy link
Member

Jontem commented Aug 29, 2018

We could make use of process.binding('natives') which returns an object with all native modules as keys

@ljani
Copy link
Contributor

ljani commented Aug 29, 2018

@Jontem Thanks for the idea!

EDIT: It seems that process.binding() has been deprecated lately.

@Jontem Jontem closed this as completed in #60 Sep 2, 2018
Jontem pushed a commit that referenced this issue Sep 2, 2018
This commit makes this resolver behave like Node does [0]:
"Core modules are always preferentially loaded if their identifier is passed to require()"

Additionally this increases performance by avoiding unnecessary disk access. For example webpack calls require("crypto") in a loop.

This commit fixes #56.

[0]: https://nodejs.org/api/modules.html#modules_core_modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants