search

lucacasonato / esbuild_deno_loader

Deno module resolution for `esbuild`
https://deno.land/x/esbuild_deno_loader
MIT License
162 stars 43 forks source link

feat: support npm specifiers #40

Closed lucacasonato closed 1 year ago

lucacasonato commented 1 year ago

This commit adds support for NPM specifiers. There is the caveat that the code must previously have been cached and a local node_modules dir generated with a command like deno cache --node-modules-dir.

Closes #29

nnmrts commented 1 year ago

the code must previously have been cached and a local node_modules dir generated with a command like deno cache --node-modules-dir.

Wouldn't this be incompatible with Deno Deploy?

lucacasonato commented 1 year ago

No, Deno Deploy will support npm: specifiers & the node_modules dir.

lucacasonato commented 1 year ago

@nnmrts This is also my first pass - there is more to come to improve this :)

rsstiglitz commented 1 year ago

@lucacasonato

I see, I was just thinking out loud about Deno Deploy limitations regarding file writing and custom command flags like --node-modules-dir but I'm excited for what's to come. 😊

zookatron commented 1 year ago

Thanks @lucacasonato, great work! I actually started working on a PR for this but your version looks like it's probably more thorough than mine 😄

Two points:

  1. It looks like what you currently have here doesn't work with the WASM version of esbuild because of the build.resolve call which appears to throw Cannot read directory ".": not implemented on js on the WASM version.
  2. It's a bit disappointing that this requires a separate NPM folder specified with the "rootDir" option, it would be great if it could resolve NPM packages using the files in Deno's default location for NPM modules as returned by "deno info" (generally ~/.cache/deno/npm/registry.npmjs.org), do you think it would be possible to have a code path supporting this use case? Let me know if you would like me to contribute.
lucacasonato commented 1 year ago

@zookatron I have branches for both of these use cases. I don't want to land everything at once though. They will come in due time.

zookatron commented 1 year ago

@zookatron I have branches for both of these use cases. I don't want to land everything at once though. They will come in due time.

Sounds good! Can you help me understand the motivation for wanting to roll these out in stages? Thanks!

lucacasonato commented 1 year ago

I'm working on figuring out the external API first, then I'll open one final PR that builds on all the previous work to introduce NPM specifiers. This PR will never be merged in its current form

Ciantic commented 1 year ago

Great this is moving! Is supporting without --node-modules-dir a target also?

I hate node_modules with a passion, avoiding that directory is a big reason I'm using Deno in the first place, I want to avoid having node_modules all over my project dirs (NTFS also really sucks with small files).

lucacasonato commented 1 year ago

@Ciantic please read the comment history

Ciantic commented 1 year ago

Okay, sorry. I did read this:

No, Deno Deploy will support npm: specifiers & the node_modules dir.

But I assumed it means when you use npm specifier it generates node_modules directory. I assume it means it's either or thing.

I discovered that Deno can even generate node_modules just couple of days ago, when I use the flag.

lucacasonato commented 1 year ago

Closed in favour of #67