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

npm: specifiers still use version in lockfile when updating config #20868

Closed
marvinhagemeister opened this issue Oct 10, 2023 · 12 comments
Closed
Assignees
Labels
bug Something isn't working correctly important for fresh

Comments

@marvinhagemeister
Copy link
Contributor

marvinhagemeister commented Oct 10, 2023

I'm running into a bug with Fresh when trying to switch over to npm: specifiers. When using both preact-render-to-string and preact Deno loads multiple copies of Preact. This breaks preact's core option API which relies on a signleton.

It seems like anything I set in the import map is ignored. There I specifically request [email protected], but somehow [email protected] is still getting installed by preact-render-to-string.

{
  "vendor": true,
  "imports": {
    "preact": "npm:[email protected]",
    "preact/": "npm:/[email protected]/",
    "preact-render-to-string": "npm:[email protected]"
  },
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "preact"
  }
}
Screenshot 2023-10-10 at 13 39 15

Steps to reproduce

  1. Clone https://github.com/marvinhagemeister/deno-npm-version-bug
  2. Run deno run foo.tsx

It will print it works when the bug is resolved, otherwise it will print it doesn't work if the bug is still present.

@marvinhagemeister marvinhagemeister added bug Something isn't working correctly important for fresh labels Oct 10, 2023
@nayeemrmn
Copy link
Collaborator

After deleting my DENO_DIR, ./deno.lock and ./node_modules it seems only 10.18.1 is included (though the repro program is still printing it doesn't work).

Before that I could reproduce, and saw the following in deno.lock:

{
  "packages": {
    "specifiers": {
      "npm:[email protected]": "npm:[email protected][email protected]",
      // ...
    }
    // ...
  }
}

Not sure what that mechanism is.

@marvinhagemeister
Copy link
Contributor Author

That makes it sound like there is a stale cache somewhere. Maybe it stores the dependencies at the time the module was downloaded or something.

@dsherret
Copy link
Member

dsherret commented Oct 11, 2023

preact-render-to-string is locked to preact 10.15.1:

https://github.com/marvinhagemeister/deno-npm-version-bug/blob/dbcaf16db6222d5f13dbd4b71b4870d0d265dfd2/deno.lock#L13

Clearing out the lockfile after upgrading dependencies is probably the best thing to do without BYONM. Perhaps maybe the lockfile could be invalidated when adding or changing a top level dependency, though that might be hard to figure out.

@marvinhagemeister
Copy link
Contributor Author

Just ran into this again while trying to use more npm: specifiers in Fresh. It seems like this is a blocker for Fresh to be able to fully move over to them.

@dsherret
Copy link
Member

I think when we see a specifier we haven't seen before, we should just invalidate the lockfile, but maintain the collection of npm specifiers to package name and version (not id with peer deps). I can look into this today.

@dsherret dsherret self-assigned this Nov 28, 2023
@marvinhagemeister
Copy link
Contributor Author

Yeah, I can't think of any other solution myself. Like you said, it seems like when we add a new npm dep we need to re-evaluate the dependency graph and re-create the lockfile to make peer dependencies work.

@dsherret
Copy link
Member

One other thing we could do is give precedence to npm specifiers in the import map. If one of them changes then we can assume that's the reference used in the project, remove it from the lockfile, then invalidate the other parts of the lockfile. That would require storing these specifiers in the lockfile though, but it would handle the scenario where you switch back and forth between a dep version.

@guillaume86
Copy link

Not sure it's the same issue but I'm trying to override a "ws" import inside the "puppeteer-core" package wich is a dependency of my direct dependency "npm:[email protected]".

Is this supposed to work? Or should I use npm: specifier somewhere? Or version specifier?

  "imports": {
    "puppeteer": "npm:[email protected]",
    "ws": "./patched/ws.ts"
  },

@dsherret
Copy link
Member

dsherret commented Jan 31, 2024

@guillaume86 no, that is #18191

In the meantime, there are two ways to get around this. One is using "nodeModulesDir": true, running deno cache main.ts, then manually patching the contents in the node_modules folder. The alternative is to use the unstable "byonm" feature (https://deno.com/blog/v1.38#nodejs-compatibility-improvements) where you need to use a package.json and install dependencies with npm.

@luizfilipe
Copy link

luizfilipe commented Mar 18, 2024

I'm currently facing a similar issue on a try to override a dependency from a my project's dependency

I tried to use this, to fix an issue caused by the latest version of reestructure in my project, as per docs , but it does not seem to behave as expected

"scopes": {
  "[email protected]": {
    "restructure": "npm:[email protected]"
  }
}

I've also tried to vendor it, setting "vendor": true, on my deno.json file.

@dsherret
Copy link
Member

@luizfilipe depend on what "fontkit" is, I think that's #18191

@dsherret dsherret changed the title npm: specifiers ignore import map npm: specifiers still use version in lockfile when updating config Mar 18, 2024
@dsherret
Copy link
Member

Also, I think this issue original issue was essentially fixed by #22004. Going to close this now, but maybe let's open a targeted issue about anything remaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly important for fresh
Projects
None yet
Development

No branches or pull requests

5 participants