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

perf(esm-loader): don't parse url if specifier starts with node: #3633

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

paul-soporan
Copy link
Member

What's the problem this PR addresses?

The ESM loader's resolve hook can skip parsing the URL if the specifier starts with node:.

Addresses #3603 (comment).

How did you fix it?

Extracted isBuiltinModule from applyPatch into nodeUtils and made the ESM loader's resolve hook use it rather than only checking Module.builtinModules.

I also changed the order of the lhs and rhs of isBuiltinModules so that it doesn't have to check the entire set first if the specifier starts with node:.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis
Copy link
Member

arcanis commented Oct 25, 2021

I also changed the order of the lhs and rhs of isBuiltinModules so that it doesn't have to check the entire set first if the specifier starts with node:.

I doubt that makes it faster; in practice very few imports will use the node: protocol, so running the .has first is more likely to return the expected result (without having to process the string first to check for a string we know won't be here). That said, I suspect it has no real bearing either way (since non-node imports are the overwhelming majority of imports and will require both calls anyway), so it's just a nit 🙂

@arcanis arcanis merged commit 4262ec1 into master Oct 25, 2021
@arcanis arcanis deleted the paul/perf/esm-loader branch October 25, 2021 08:33
@merceyz merceyz added the esm label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants