-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(node): improve cjs tracking #22673
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests pass and a test is there so this is probably all fine. Let some comments and thoughts.
sloppy_imports_resolver: Option<SloppyImportsResolver>, | ||
mapped_specifier_resolver: MappedSpecifierResolver, | ||
maybe_default_jsx_import_source: Option<String>, | ||
maybe_jsx_import_source_module: Option<String>, | ||
maybe_vendor_specifier: Option<ModuleSpecifier>, | ||
cjs_resolutions: Option<Arc<CjsResolutionStore>>, | ||
node_resolver: Option<Arc<NodeResolver>>, | ||
node_resolver: Option<Arc<CliNodeResolver>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: It seems odd that CliNodeResolver
is inside an Arc while each of its fields is also an Arc
. One or the other should be enough I believe, though I am not quite sure. @mmastrac might probably have a clearer view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly all of these "service" structs are in an Arc. I think it's more convenient because then you don't need to go around refactoring a bunch of code when the internals change and it's a simple pattern to just do and not worry about. Also, I guess in this case it's a question of whether it's faster to clone 4 Arcs when cloning a CliNodeResolver during initialization vs adding more indirection when using a CliNodeResolver. Probably this is accessed way more than cloned, but my guess would be that it's a micro-optimization that doesn't matter, but maybe not.
// If so, check if we need to store this specifier as being a CJS | ||
// resolution. | ||
let specifier = | ||
crate::node::resolve_specifier_into_node_modules(&specifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: There seems to be a fairly significant amount of cloning happening hereabouts.
resolve_specifier_into_node_modules
always clones but it seems like it might do extra clones as well. url_to_node_resolution
below presumably also clones. Inserting into cjs_resolutions
also clones (this also seems like it might be removable).
It's probably not a real issue but might be worth a glance somewhere down the line if performance in node resolution is found to be somewhat lacking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolve_specifier_into_node_modules
seems to conditionally clone? url_to_node_resolution
does do a to_lowercase to check the extension, which is a clone that could be removed and do a case insensitive comparison. I'm not sure how the clone inserting into cjs_resolutions is removable? Is there a design that could prevent that?
We were missing saying that a file is CJS when some Deno code imported from the node_modules directory at runtime.
We were missing saying that a file is CJS when some Deno code imported from the node_modules directory at runtime. Ran into this using vite while upgrading ts-ast-viewer.com to use Deno w/ byonm.