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

Ability to resolve npm specifier imports in bundlers made for npm #17043

Closed
itsdouges opened this issue Dec 13, 2022 · 12 comments
Closed

Ability to resolve npm specifier imports in bundlers made for npm #17043

itsdouges opened this issue Dec 13, 2022 · 12 comments
Labels
needs discussion this topic needs further discussion to determine what action to take node compat

Comments

@itsdouges
Copy link

itsdouges commented Dec 13, 2022

Hi! As mentioned in #15960 I'm opening a new issue to talk about this issue I'm having.

Thanks to #15427 and npm: specifiers hitting stable you can run Vite in Deno which works. One issue is that for any dependencies in the frontend graph that aren't executed in Deno they don't ever get their dependencies resolved/cached.

Because of this you have to rely on importing them in any file that is executed in Deno, e.g. in the React example here https://deno.land/manual@v1.28.2/node/how_to_with_npm/react#add-a-router you show importing the dependencies used in the frontend app in the config file.

This will then "install" the dependencies on run, and then make them available in node_modules for Vite (and any other bundler). This unblocks usage but causes a few problems:

  1. Without importing the dependencies in a module ran in Deno they'll never be installed
  2. The frontend app can't use npm: specifiers, but instead the node_module name (react instead of npm:react)
  3. When using the node_module name (react instead of npm:react) they aren't type checked (because Deno doesn't know about the module). So you need to resort to creating a import_map.json, example below:
  4. When setting jsxImportSource to react it will add to the source file the react/jsx-runtime import... but of course that isn't a real dependency for Deno so it's another entry to add to the import_map
  "imports": {
    "@faze/editor-backend": "./packages/editor-backend/mod.ts",
    "@faze/editor-cli": "./packages/editor-cli/mod.ts",
    "@faze/editor-frontend": "./packages/editor-frontend/mod.ts",
    "@react-three/drei": "npm:@react-three/drei@9.45.0",
    "@react-three/fiber": "npm:@react-three/fiber@8.9.1",
    "react-dom": "npm:react-dom@18.2.0",
    "react-dom/client": "npm:react-dom@18.2.0/client",
    "react-router-dom": "npm:react-router-dom@6.4.4",
    "react": "npm:react@18.2.0",
    "react/jsx-runtime": "npm:react@18.2.0/jsx-runtime",
    "three": "npm:three@0.144.0",
    "three-stdlib": "npm:three-stdlib@2.20.4"
  }
}

Doing all of the above at the very least gets it into a usable state but it's not ideal. If we could resolve Deno npm specifier imports (and during resolution it installs/caches them) I can imagine a plugin being written for bundlers to make use of it so it has better interop.

I will say the interop between a natively installed npm package (via npm, and hopefully other package managers) and Deno npm:specifiers worked suprisingly well and the two seem to work together with each other, very great stuff.

Let me know if you need more info 😄.

@bartlomieju
Copy link
Member

Replying here from the thread in #15427

Thanks, it's Rollup but same same. Is there a JS api for info/cache or it's only CLI? (JS would be useful here). I imagine this would be, for each import found that is probably from node modules, find package json, find dependency, find version, and then hit Deno APIs, eventually resolving to the cache location for said dependency.

Errr, yeah I mean rollup. It's only CLI and we want to keep it that way. I suggest to just leave all the resolution to Deno without meddling with package.json yourself - it should be more reliable that way (avoiding discrepancies how Deno discovers dependencies and how your code would do it).

I've been toying with the shipping a storybook like experience with Deno + Vite, so this is a big hurdle I need to jump over to get it working. Any thoughts would be appreciated. I also have an issue open here about this: #17043

Yes, let's move the discussion on this particular topic to that issue.

Looks deno info isn't showing local cache location for npm modules:

I discussed it with @dsherret yesterday, and indeed the cache path is only specified in non-JSON output, we will most likely add path to the cached sources to the JSON output. Have you started any work on the plugin already?

@itsdouges
Copy link
Author

itsdouges commented Dec 17, 2022

Hi! Ok so looking at deno info a bit more it might work pretty well.

➜  ~ deno info --json
{
  "denoDir": "/Users/douges/Library/Caches/deno",
  "modulesCache": "/Users/douges/Library/Caches/deno/deps",
  "npmCache": "/Users/douges/Library/Caches/deno/npm",
  "typescriptCache": "/Users/douges/Library/Caches/deno/gen",
  "registryCache": "/Users/douges/Library/Caches/deno/registries",
  "originStorage": "/Users/douges/Library/Caches/deno/location_data"
}
➜

That has the information I need to find the module (as for npm it will just be {npmpath}/{name}@{version}. I was surprised at the output difference of deno info npm:react and deno info npm:react --json - is the latter meant to show all npm dependencies with that use the specified one I guess?

Have you started any work on the plugin already?

I'm currently playing around with something locally to get it working. When I have an initial pass I'll put something on Github/deno_x. Is there any documentation or code references for how Deno currently resolves npm specifiers (with dependencies).

@itsdouges
Copy link
Author

itsdouges commented Dec 17, 2022

Will import.meta.resolve be implemented for npm specifiers? It would simplify a bit, at least for cache lookup, as I'd be able to lean on Deno's internal resolution instead of trying to re-implement it myself. I don't see much movement in #16089 ATM.

Few things to figure out for getting a bundler plugin working:

  1. Figuring out the version to look for (e.g. npm:react should implicitly resolve latest, but any of its dependencies inside its modules would need to be inferred from pkg json)
  2. Figuring out what entry point to use - main (cjs), module (esm), browser, and so on. Not to mention nested ones e.g. react/jsx-runtime. Realistically would need to follow the config in vite/rollup.
  3. Resolving cache location, mostly easy enough by a combination of deno cache + deno info, but knowing what registry folder would be tricky if using a custom one. For now I'd just hard code the default.

I noticed you've tried writing a rollup plugin, how did you go? #16089 (comment)

@bartlomieju
Copy link
Member

Hi! Ok so looking at deno info a bit more it might work pretty well.

➜  ~ deno info --json
{
  "denoDir": "/Users/douges/Library/Caches/deno",
  "modulesCache": "/Users/douges/Library/Caches/deno/deps",
  "npmCache": "/Users/douges/Library/Caches/deno/npm",
  "typescriptCache": "/Users/douges/Library/Caches/deno/gen",
  "registryCache": "/Users/douges/Library/Caches/deno/registries",
  "originStorage": "/Users/douges/Library/Caches/deno/location_data"
}
➜

That has the information I need to find the module (as for npm it will just be {npmpath}/{name}@{version}. I was surprised at the output difference of deno info npm:react and deno info npm:react --json - is the latter meant to show all npm dependencies with that use the specified one I guess?

I'm not sure why there's a difference. @dsherret could you explain it? That said we should add complete path to cached module in the JSON output.

Have you started any work on the plugin already?

I'm currently playing around with something locally to get it working. When I have an initial pass I'll put something on Github/deno_x. Is there any documentation or code references for how Deno currently resolves npm specifiers (with dependencies).

No, there's no such documentation and I strongly feel it should remain a black box - otherwise if we need to change or fix something the plugins relying on this will break, whilst if they shell out to deno itself they will have consistent behavior.

Will import.meta.resolve be implemented for npm specifiers? It would simplify a bit, at least for cache lookup, as I'd be able to lean on Deno's internal resolution instead of trying to re-implement it myself. I don't see much movement in #16089 ATM.

Unclear at the moment - import.meta.resolve() as per spec can return empty result (or throw an error) if a file hasn't been cached/loaded. So that would be surprising, while deno info will ensure that package is downloaded and cached before the data is returned.

Few things to figure out for getting a bundler plugin working:

Figuring out the version to look for (e.g. npm:react should implicitly resolve latest, but any of its dependencies inside its modules would need to be inferred from pkg json)

That's already handled by various subcommands (like deno cache and deno info so shouldn't be a problem).

Figuring out what entry point to use - main (cjs), module (esm), browser, and so on. Not to mention nested ones e.g. react/jsx-runtime. Realistically would need to follow the config in vite/rollup.

I think plugins should only use deno info to resolve the package itself - the entry point or subpath should be handled be plugin as it is a well known algorithm described in Node docs (and IIRC the default rollup resolver can already do that).

Resolving cache location, mostly easy enough by a combination of deno cache + deno info, but knowing what registry folder would be tricky if using a custom one. For now I'd just hard code the default.

That's reasonable - with latest version deno respects NPM_REGISTRY_CONFIG env var which is inline with npm's behavior.

I noticed you've tried writing a rollup plugin, how did you go? #16089 (comment)

When I was working on it we didn't have support for npm specifier in deno info so I hard coded most of the logic then, it worked reasonable well, but obviously it's better not to duplicate this logic and now with deno info support it's feasible to do.

Let me know when you get something published, I can give you a hand smoothing out problematic parts.

@bartlomieju bartlomieju added needs discussion this topic needs further discussion to determine what action to take node compat labels Dec 18, 2022
@itsdouges
Copy link
Author

One of the difficult parts I'm having right now is resolving nested entry points. Atm I'm getting the Deno cache absolute location of a npm pkg and then calling this.resolve to get the node resolver to do the heavy lifting. It works, until I encounter a nested entry point, and then I'm at a loss for how to make it work without reimplementing node resolver, specifically the pkg json exports logic.

When you were writing your plugin did you do anything special to handle this? Basically I'd love the Deno plugin to only care about two things:

  1. Enforcing Deno semantics for https/fs imports
  2. Finding the Deno cache location for a npm import and then passing it onto the node resolver

But (2) I'm finding difficult.

@bartlomieju
Copy link
Member

bartlomieju commented Dec 18, 2022

One of the difficult parts I'm having right now is resolving nested entry points. Atm I'm getting the Deno cache absolute location of a npm pkg and then calling this.resolve to get the node resolver to do the heavy lifting. It works, until I encounter a nested entry point, and then I'm at a loss for how to make it work without reimplementing node resolver, specifically the pkg json exports logic.

When you were writing your plugin did you do anything special to handle this? Basically I'd love the Deno plugin to only care about two things:

  1. Enforcing Deno semantics for https/fs imports
  2. Finding the Deno cache location for a npm import and then passing it onto the node resolver

But (2) I'm finding difficult.

@itsdouges could you give an example of such entry point? Maybe I'm missing something here.

Also it feels I should start a new repository for a rollup plugin and we could work from there?

@itsdouges
Copy link
Author

Ones such as @babel/runtime/helpers/AsyncGenerator. It's easy enough to find the location of @babel/runtime folder in the Deno cache and pass it as an absolute path to the node resolver plugin via this.resolve, but when it's a nested entry point like above I'm at a loss if it's still possible to pass an absolute path...

@bartlomieju
Copy link
Member

So doing deno info deno info npm:@babel/runtime/helpers/AsyncGenerator can be successfully resolved, it's just a matter of not having a cache path to that concrete file. I will look into this problem this week. Thanks for pushing this forward @itsdouges, it's very valuable!

@itsdouges
Copy link
Author

Sweet thank you mate, that would provide a way forward 😊

@itsdouges
Copy link
Author

@bartlomieju is there any issue I can follow for successfully resolving the cache location of npm dependencies?

  • npm:@babel/core
  • npm:@babel/core@1.2.1
  • npm:@babel/runtime/helpers/AsyncGenerator
  • npm:@babel/runtime@1.3.2/helpers/AsyncGenerator

And so on.

@bartlomieju
Copy link
Member

@bartlomieju is there any issue I can follow for successfully resolving the cache location of npm dependencies?

  • npm:@babel/core
  • npm:@babel/core@1.2.1
  • npm:@babel/runtime/helpers/AsyncGenerator
  • npm:@babel/runtime@1.3.2/helpers/AsyncGenerator

And so on.

Opened: #17168

@lucacasonato
Copy link
Member

This is now fixed because deno install / deno cache now install NPM and JSR deps found in package.json and deno.json imports, even if they are not imported in Deno TS code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion this topic needs further discussion to determine what action to take node compat
Projects
None yet
Development

No branches or pull requests

3 participants