denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.22k stars 5.37k forks source link

Deleted dependencies from earlier version of the code still downloaded #21261

Open marvinhagemeister opened 11 months ago

marvinhagemeister commented 11 months ago

Just noticed that deno run downloads a lot of modules referencing an earlier iteration of the code when deno.json is present. When I delete the deno.json file it works as expected.

Steps to reproduce

  1. Create deno.json with { "lock": true } as the content
  2. Create `foo.ts with this content:
    import "npm:htm";
    import "npm:preact";
    import "npm:preact-render-to-string";
    console.log("foo");
  3. Run deno run -A foo.ts (a deno.lock should be present now)
  4. Delete all imports in foo.ts
  5. Run deno run -Ar foo.ts -> Downloads all npm modules from the previous iteration

Tip: On macOS/Linux use deno run -Ar foo.ts &> foo.log to pipe the output to foo.log which highlights the problem more clearly.

It seems like it's not possible to remove an npm dependency once it was imported at some point in the project. Looking at the deno.lock file it seems to confirm that theory as it still contains all three npm imports.

dsherret commented 11 months ago

At the moment, Deno always initializes npm packages found in a lockfile on startup when not using byonm. When --node-modules-dir=false, maybe we could lazily download only the used subset of the tarballs, but it probably should still initialize resolution from the lockfile because otherwise the implementation would get very complex. Npm resolution is based on all the packages used and seemingly unrelated packages can affect the resolution of other packages due to their shared descendant dependencies.

That said, an issue with changing the behaviour to lazily download npm tarballs will be that it causes npm packages to be downloaded midway through execution by a previously executed unanalyzable dynamic import, instead of upfront like it currently does. This will cause a slowdown of the code being executed and could cause issues if it fails to download the tarballs while already deployed instead of potentially pre-deployment like before. This also raises the question of whether deno cache mod.ts should have different behaviour than deno run mod.ts... I don't believe they should differ.

I don't believe we can do this optimization when using a node_modules directory without BYONM. It's important for Deno to initialize the node_modules folder on startup because the code being executed might depend on the node_modules folder existing and sometimes packages depend on other packages which might not even specify those packages as depenencies (so I believe doing that optimization with a node_modules folder would cause bugs).

A benefit and drawback with Deno's approach is that the source of truth for dependencies is not the package.json, but any module file that could be executed (this is great for https imports, but not so much for npm imports). For realizing that the npm dependencies are no longer used in the project, a potential solution might be to keep track of usages of npm packages in local and remote modules and store that in the lockfile, but this is complicated to do and I'm not sure it would work reliably (ex. we'd have to handle file renames and deletions). Additionally, we also currently store binary commands executed on the command line in the lockfile (so if you use an npm package as a task or in a script then they'll be locked in the lockfile. This also has the benefit of being faster on subsequent runs).

So overall, I'm not sure we can optimize the current design without causing potential issues. Maybe we need to come up with other solutions to this, such as making it easy to reset a lockfile or have a strict mode where all dependencies need to be defined in the deno.json. In this regard, npm install is better for only installing the packages specified in a package.json (and the new byonm feature helps a lot here with that).

marvinhagemeister commented 11 months ago

I see. It's a bit unfortunate that the lockfile is going out of sync.

FWIW the same issue occurs with http imports. It's not unique to npm: prefixes.

filipe-freire commented 2 months ago

Hey there! I know this issue is >1y old, but I still faced it today. I know it's not ideal, but here's my workaround for this issue:

  1. Delete the unused package mentions from your codebase
  2. Disable the Deno extension on your IDE (to prevent it from eagerly generating a lock file)
  3. Delete the deno.lock file
  4. Enable the Deno extension again
  5. Profit! 💰

This forces a synced deno.lock as far as I could test :)