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

Really inefficient resolution #116

Closed
steelbrain opened this issue Jan 26, 2017 · 7 comments · Fixed by #190
Closed

Really inefficient resolution #116

steelbrain opened this issue Jan 26, 2017 · 7 comments · Fixed by #190

Comments

@steelbrain
Copy link

steelbrain commented Jan 26, 2017

Suppose we are in /a/b/c/d.js and require some-thing and node_modules is in /a/node_modules, node-resolve handles this extremely efficiently

Current behavior:

Checks these in order to see which one exists

  • /a/b/c/node_modules/some-thing.js
  • /a/b/c/node_modules/some-thing/node_modules/package.json
  • /a/b/node_modules/some-thing.js
  • /a/b/node_modules/some-thing/node_modules/package.json
  • /a/b/c/node_modules/some-thing.js
  • /a/b/c/node_modules/some-thing/node_modules/package.json
  • /a/node_modules/some-thing.js
  • /a/node_modules/some-thing/node_modules/package.json

It gets even more inefficient as the number of possible extension grows, it does some many stats that for our module bundler bundling our app it can easily get to a hundred thousand unnecessary stats

The ideal behavior would be

  • /a/b/c/node_modules
  • /a/b/node_modules
  • /a/node_modules
  • /a/node_modules/some-thing.js
  • /a/node_modules/some-thing/node_modules/package.json

I'm on resolve@1.2.0 at the time of writing

@ljharb
Copy link
Member

ljharb commented Jan 26, 2017

I'm afraid this is the actual resolve algorithm used by node - its efficiency is irrelevant; its correctness is what's important, and node isn't going to change it.

@ljharb ljharb closed this as completed Jan 26, 2017
@steelbrain
Copy link
Author

Hi @ljharb I respect prioritizing correctness.

but if you look at my examples above, you would see that correctness doesn't change. I might not have been able to explain it properly but what I'm asking for is

This resolve should first check for the existance of the node_modules directory before making say 4-5 or more stat calls within that directory depending on the number of extensions

@ljharb
Copy link
Member

ljharb commented Jan 26, 2017

aha, I misunderstood.

You're saying that in the case where none of the paths in that list exists but the last one, it should be more efficiently reaching the last one?

@ljharb ljharb reopened this Jan 26, 2017
@steelbrain
Copy link
Author

@ljharb Yes, in most of the cases node_modules doesn't exist in upto 3-4 parent dirs, if the resolve function makes sure that node_modules exists in that directory before trying to resolve from it then that would save us a lot of stat calls (and wouldn't compromise correctness)

@ljharb
Copy link
Member

ljharb commented Jan 26, 2017

That seems reasonable.

@steelbrain
Copy link
Author

@ljharb I know you're a busy man but pinging to see if you could work on this sometime soon as it would make a noticeable performance improvement for all dependents of this module

@ljharb
Copy link
Member

ljharb commented May 10, 2018

I'd happily review a PR for it, but I'm not likely to have time in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants