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

feat(unstable): support caching npm dependencies only as they're needed #27300

Merged
merged 13 commits into from
Dec 11, 2024

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Dec 10, 2024

Currently deno eagerly caches all npm packages in the workspace's npm resolution. So, for instance, running a file foo.ts that imports npm:chalk will also install all dependencies listed in package.json and all npm dependencies listed in the lockfile.

This PR refactors things to give more control over when and what npm packages are automatically cached while building the module graph.

After this PR, by default the current behavior is unchanged except for deno install --entrypoint, which will only cache npm packages used by the given entrypoint. For the other subcommands, this behavior can be enabled with --unstable-npm-lazy-caching

TODO:

  • bikeshed the unstable flag name
  • add some more tests?

Fixes #25782.

@nathanwhit nathanwhit added the ci-draft Run the CI on draft PRs. label Dec 10, 2024
@nathanwhit nathanwhit marked this pull request as ready for review December 10, 2024 04:42
@nathanwhit nathanwhit removed the ci-draft Run the CI on draft PRs. label Dec 10, 2024
@nathanwhit nathanwhit force-pushed the package-json-autoinstall branch from e3ada55 to 03c5334 Compare December 10, 2024 17:40
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks really good. Just have a few questions.

cli/graph_util.rs Outdated Show resolved Hide resolved
GraphKind::All,
roots.clone(),
&mut loader,
graph_util::NpmCachingStrategy::Eager,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like maybe we could maybe get away with setting this as lazy in the lsp or just only install stuff found in the package.json, deno.json, or any specifiers.

cli/npm/managed/mod.rs Show resolved Hide resolved
cli/args/flags.rs Outdated Show resolved Hide resolved
@nathanwhit nathanwhit force-pushed the package-json-autoinstall branch from b7c7b2a to 9bc0da0 Compare December 10, 2024 22:50
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nathanwhit nathanwhit merged commit 6f50620 into denoland:main Dec 11, 2024
17 checks passed
@nathanwhit nathanwhit deleted the package-json-autoinstall branch December 11, 2024 02:24
bartlomieju pushed a commit that referenced this pull request Dec 11, 2024
…ed (#27300)

Currently deno eagerly caches all npm packages in the workspace's npm
resolution. So, for instance, running a file `foo.ts` that imports
`npm:chalk` will also install all dependencies listed in `package.json`
and all `npm` dependencies listed in the lockfile.

This PR refactors things to give more control over when and what npm
packages are automatically cached while building the module graph.

After this PR, by default the current behavior is unchanged _except_ for
`deno install --entrypoint`, which will only cache npm packages used by
the given entrypoint. For the other subcommands, this behavior can be
enabled with `--unstable-npm-lazy-caching`


Fixes #25782.

---------

Signed-off-by: Nathan Whitaker <[email protected]>
Co-authored-by: Luca Casonato <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deno install sometimes calls sync_resolution_with_fs multiple times
3 participants