-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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(npm): resolve dynamic npm imports individually #24170
fix(npm): resolve dynamic npm imports individually #24170
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.
Main:
---- specs::npm::dynamic_npm_resolution_failure ----
command V:\deno\target\debug\deno.exe run -A main.ts
command cwd V:\deno\tests\specs\npm\dynamic_npm_resolution_failure
output path V:\deno\tests\specs\npm\dynamic_npm_resolution_failure\main.out
-- OUTPUT START --
Download http:https://localhost:4260/chalk
Download http:https://localhost:4260/@denotest/dep-cannot-parse
error: Error in @denotest/[email protected] parsing version requirement for dependency: @denotest/esm-basic@unknown-scheme:unknown
Invalid npm version requirement. Unexpected character.
unknown-scheme:unknown
~
Caused by:
0: Invalid npm version requirement. Unexpected character.
unknown-scheme:unknown
~
1: Unexpected character.
unknown-scheme:unknown
~
@@ -771,9 +763,7 @@ fn resolve_package_json_dep( | |||
#[derive(Debug)] | |||
pub struct WorkerCliNpmGraphResolver<'a> { | |||
npm_resolver: Option<&'a Arc<dyn CliNpmResolver>>, | |||
// use a cache per worker so that another worker doesn't clear the | |||
// cache of another worker | |||
memory_cache: Rc<RefCell<HashMap<String, Arc<NpmPackageInfo>>>>, |
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.
After some other refactors, this cache is only a bit of an optimization when many workers are doing npm resolution and the complexity is not worth keeping around for that edge case (it will still use a cache, but it will be a file system one).
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.
LGTM, this is a really great change 👍
Fixes a regression introduced in #24170, where we wouldn't actually set up the node modules dir on `deno install` if there was an up to date deno lockfile present. Previously we were relying on the fact that resolving pending module resolution called `cache_packages` (which sets up the node modules dir). When pending resolutions were removed, and the `resolve_pending` function with it, we also removed the `cache_packages` call needed to set up node modules.
Fixes a regression introduced in #24170, where we wouldn't actually set up the node modules dir on `deno install` if there was an up to date deno lockfile present. Previously we were relying on the fact that resolving pending module resolution called `cache_packages` (which sets up the node modules dir). When pending resolutions were removed, and the `resolve_pending` function with it, we also removed the `cache_packages` call needed to set up node modules.
…nd#24190) Fixes a regression introduced in denoland#24170, where we wouldn't actually set up the node modules dir on `deno install` if there was an up to date deno lockfile present. Previously we were relying on the fact that resolving pending module resolution called `cache_packages` (which sets up the node modules dir). When pending resolutions were removed, and the `resolve_pending` function with it, we also removed the `cache_packages` call needed to set up node modules.
Closes #17802