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

Improve legacy CJS resolve #73

Closed
anonrig opened this issue Apr 13, 2023 · 5 comments
Closed

Improve legacy CJS resolve #73

anonrig opened this issue Apr 13, 2023 · 5 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed performance-agenda

Comments

@anonrig
Copy link
Member

anonrig commented Apr 13, 2023

The following code calls multiple C++ calls for each new URL and fileExists function call. We can avoid these changes, make 1 C++ call, and handle the rest in the C++ layer.

function legacyMainResolve(packageJSONUrl, packageConfig, base) {
  let guess;
  if (packageConfig.main !== undefined) {
    // Note: fs check redundances will be handled by Descriptor cache here.
    if (fileExists(guess = new URL(`./${packageConfig.main}`,
                                   packageJSONUrl))) {
      return guess;
    } else if (fileExists(guess = new URL(`./${packageConfig.main}.js`,
                                          packageJSONUrl)));
    else if (fileExists(guess = new URL(`./${packageConfig.main}.json`,
                                        packageJSONUrl)));
    else if (fileExists(guess = new URL(`./${packageConfig.main}.node`,
                                        packageJSONUrl)));
    else if (fileExists(guess = new URL(`./${packageConfig.main}/index.js`,
                                        packageJSONUrl)));
    else if (fileExists(guess = new URL(`./${packageConfig.main}/index.json`,
                                        packageJSONUrl)));
    else if (fileExists(guess = new URL(`./${packageConfig.main}/index.node`,
                                        packageJSONUrl)));
    else guess = undefined;
    if (guess) {
      emitLegacyIndexDeprecation(guess, packageJSONUrl, base,
                                 packageConfig.main);
      return guess;
    }
    // Fallthrough.
  }
  if (fileExists(guess = new URL('./index.js', packageJSONUrl)));
  // So fs.
  else if (fileExists(guess = new URL('./index.json', packageJSONUrl)));
  else if (fileExists(guess = new URL('./index.node', packageJSONUrl)));
  else guess = undefined;
  if (guess) {
    emitLegacyIndexDeprecation(guess, packageJSONUrl, base, packageConfig.main);
    return guess;
  }
  // Not found.
  throw new ERR_MODULE_NOT_FOUND(
    fileURLToPath(new URL('.', packageJSONUrl)), fileURLToPath(base));
}
@aduh95
Copy link

aduh95 commented Apr 13, 2023

Does it matter though? I'd expect that packages that do not define a "main" nor "exports" field in their package.json is rather rare – and maybe you could/should measure that before working on this.
EDIT: I misread the code, there might still be lots of cases where packages define a "main" but without an extension, in which case it might indeed be worth trying to optimize it.

@aduh95
Copy link

aduh95 commented Apr 13, 2023

Not related to performance, but another problem with that function is that it seems to assume that "main" contains a URL-compatible string, when other part of the codebase (and the docs) treats it as a path.

@metcoder95
Copy link
Member

metcoder95 commented Apr 18, 2023

Then we have 2 separate issues?
Standardize its usage, and make optimizations out of it, right?

Not sure the former relates to performance but we can work at both ends, should this issue reflect that?

@H4ad
Copy link
Member

H4ad commented May 29, 2023

I started working on this issue, I will try more ideas and then share when the code is more mature.

@H4ad
Copy link
Member

H4ad commented Jun 3, 2023

Hey, I created a PR for NodeJS with current work: nodejs/node#48325

I'm almost done, the performance improvements have been really nice and also bring the possibility to improve other parts of the system like fileURLToPath.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed performance-agenda
Projects
None yet
Development

No branches or pull requests

5 participants