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.

Sign up for GitHub

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

Jump to bottom

fix(npm): resolve dynamic npm imports individually #24170

Merged
merged 7 commits into from
Jun 11, 2024
Merged

fix(npm): resolve dynamic npm imports individually #24170

merged 7 commits into from
Jun 11, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret mentioned this pull request Jun 10, 2024
Closed
dsherret
dsherret commented Jun 10, 2024
View reviewed changes
tests/specs/npm/dynamic_npm_resolution_failure/main.out Outdated
Copy link
Member Author

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://localhost:4260/chalk
Download http://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
         ~

@dsherret dsherret marked this pull request as ready for review June 10, 2024 23:38
dsherret
dsherret commented Jun 10, 2024
View reviewed changes
cli/resolver.rs
@@ -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>>>>,
Copy link
Member Author

@dsherret dsherret Jun 10, 2024 •
edited
Loading

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).

bartlomieju
bartlomieju approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a 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 👍

@dsherret dsherret merged commit 4bc96c5 into denoland:main Jun 11, 2024
17 checks passed
@dsherret dsherret deleted the fix_resolve_dynamic_npm_imports_individually branch June 11, 2024 12:55
@nathanwhit nathanwhit mentioned this pull request Jun 12, 2024
Merged
nathanwhit added a commit that referenced this pull request Jun 12, 2024
@nathanwhit
fix(cli): Explicitly cache NPM packages during deno install (#24190)
a753136 
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.
nathanwhit pushed a commit that referenced this pull request Jun 13, 2024
@dsherret @nathanwhit
fix(npm): resolve dynamic npm imports individually (#24170)
7cf15a4 
* denoland/deno_npm#57
* denoland/deno_graph#498

Closes #17802
nathanwhit added a commit that referenced this pull request Jun 13, 2024
@nathanwhit
fix(cli): Explicitly cache NPM packages during deno install (#24190)
4af7fb8 
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.
zebreus pushed a commit to zebreus/deno that referenced this pull request Jul 8, 2024
@nathanwhit @zebreus
fix(cli): Explicitly cache NPM packages during deno install (denola…
965828a 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynamic npm module imports should be able to fail individually #17770
2 participants
@dsherret @bartlomieju