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

Invalid module URLs leading to HTTP 404/504 produced when using new URL(…, import.meta.url) inside 3rd party module #10839

Closed
7 tasks done
wojtekmaj opened this issue Nov 8, 2022 · 13 comments

Comments

@wojtekmaj
Copy link

wojtekmaj commented Nov 8, 2022

Describe the bug

Getting module URL in our own code like so:

const module2OtherUrl = new URL("module-2/other", import.meta.url);

works fine, but if that very same line is placed in node_modules/module-1/index.js, it results in an invalid URL being generated, requesting which results in 404 Not found or 504 Gateway timeout error.

Interestingly, "module-2", "module-2/index.js" and "module-2/other.js" work, but "module-2/index" and "module-2/other" do not.

"module-2/index" and "module-2/other" also produce invalid URLs in our own code, but that's a different story.

Reproduction

https://github.com/wojtekmaj/vite-missing-deps-bug

Steps to reproduce

  • yarn
  • yarn copy-packages (to replace symlinks in node_modules with actual modules, since they come from internal workspace)
  • yarn dev
  • Observe browser console 💥

System Info

System:
    OS: macOS 13.0
    CPU: (8) arm64 Apple M1 Pro
    Memory: 457.75 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.12.0 - /usr/local/bin/node
    Yarn: 3.2.4 - /usr/local/bin/yarn
    npm: 8.19.2 - /usr/local/bin/npm
  Browsers:
    Chrome: 107.0.5304.87
    Firefox: 106.0.3
    Safari: 16.1
  npmPackages:
    vite: ^3.2.3 => 3.2.3

Used Package Manager

yarn

Logs

No response

Validations

@wojtekmaj wojtekmaj changed the title Unable to use new URL(…, import.meta.url) inside 3rd party module Invalid module URLs leading to HTTP 404/504 produced when using new URL(…, import.meta.url) inside 3rd party module Nov 9, 2022
@jonaskuske
Copy link
Contributor

jonaskuske commented Nov 9, 2022

(minimized) I don't think that resolving modules via bare import specifiers is a good idea. It's incompatible with the spec and for that reason I also think #7837 is a problem:
// file: /home/project/main.js
new URL('utils/index.js', import.meta.url)

This should resolve to /home/project/utils/index.js when run in Node or to some http URL when run in the browser, combining the relative part of the URL (1st argument) and the base (2nd argument). But if a user installs the utils package, the constructed url suddenly changes and points at a file in node_modules.

In first party code that might be acceptable, but if the transform is applied to modules, it can break 3rd party code that is valid JS.

@jonaskuske
Copy link
Contributor

jonaskuske commented Nov 9, 2022

(minimized) Potential options I can think of:
  • add a proprietary protocol, e.g. the way Parcel does it:
new URL('npm:utils/index.js', import.meta.url)
  • only resolve if 2nd argument (the base) is omitted, so the first argument needs to be absolute to be valid at runtime. This makes a bare import invalid at runtime, eliminating ambiguity.
new URL('utils/index.js')
  • transform import.meta.resolve() instead, the actual way to resolve modules URLs. (without broad support yet) But not sure if this causes additional build-time vs run-time ambiguity.

And if a module needs to resolve an absolute URL/path to a file within the same module, it can always use a valid URL (no bare imports, file extension), no transforms needed.

const runtimeUrl = new URL('./my-worker.js', import.meta.url)

@wojtekmaj
Copy link
Author

Whether or not it's spec compliant is another (important, but still another) problem. The main issue here is that the same piece of code produces different outputs, depending on file location (inside or outside node_modules).

@jonaskuske
Copy link
Contributor

jonaskuske commented Nov 9, 2022

(minimized) Hmm, maybe I'm off here, but from my understanding Vite expects dependencies to be standard, runnable JavaScript and doesn't do non-standard transforms on dep code (see also: JSX) – different output of code outside and inside `node_modules` is expected. So to me this sounds like a feature request and not a bug report.

Extending the #7837 feature in its current form to dependency code would not only allow dependencies to ship code that is bundler-specific and works in no runtime (okay, maybe?), but is non-deterministic and can lead to breakage (not okay imo).

However, I do think that there's a case for URL module imports in dependency code, so I suggested alternative transform implementations that can safely be applied to source code as well as dependency code.

@jonaskuske
Copy link
Contributor

jonaskuske commented Nov 9, 2022

Sorry, didn't notice that most of this stuff already works and it's just pre-bundling that breaks things! Minimized the respective comments :)

I think import worker from 'x?url' or import worker from 'x?worker' not working with pre-bundling is working as intended, Vite-specific transforms are meant for source code.

Not sure if new URL requiring the path to the file + extension is intended. It currently only resolves the package name to the package directory instead of doing full Node resolution, might be a bug.

But new URL breaking when a relative import is used, because it resolves to the deps directory for pre-bundled files instead of the node_modules directory, definitely sounds like a bug to me.

@bluwy
Copy link
Member

bluwy commented Dec 10, 2022

You can't use Vite specific features within third-party packages. Vite will ignore them which causes the invalid URLs. If you want to process them by Vite, you need to add the package to optimizeDeps.exclude

@bluwy bluwy closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2022
@jonaskuske
Copy link
Contributor

jonaskuske commented Dec 10, 2022

You can't use Vite specific features within third-party packages. Vite will ignore them which causes the invalid URLs. If you want to process them by Vite, you need to add the package to optimizeDeps.exclude

They aren't though! While Vite-specific imports and module resolution for new URL is tooling specific indeed, new URL('./stats.txt', import.meta.url) is standard usage and works both in browsers and in Node, still Vite breaks it when pre-bundling. So that's not Vite-specific but just a plain bug.
(and I actually have a branch with a proposed solution somewhere, gonna look for it later)

@bluwy
Copy link
Member

bluwy commented Dec 10, 2022

Vite does break it during prebundling because the library would be in a single file, and relative paths that were in node_modules are lost, so new URL('./stats.txt', import.meta.url) would be sent as-is to the browser which likely didn't work. The only way around this is to not prebundle it, I'm not sure if there's any other way except rewriting the paths 🤔

@GuskiS
Copy link

GuskiS commented Dec 12, 2022

Just encountered this myself - have library that uses new URL with relative path to load worker. It fails in vite dev mode, but works after building 🤷‍♂️

@jonaskuske
Copy link
Contributor

The only way around this is to not prebundle it, I'm not sure if there's any other way except rewriting the paths 🤔

Yeah, rewriting the path was what I was thinking about. You need to change the URL base to the package directory in node_modules, then it works:

// node_modules/some-package/workers/init.js
let conf = new URL('./config.txt', import.meta.url)

// after pre-bundling
let conf = new URL('./config.txt', '@fs/home/path/to/node_modules/some-package/workers/')

@GuskiS
Copy link

GuskiS commented Dec 13, 2022

Is there a workaround I could use to solve this problem?

@jonaskuske
Copy link
Contributor

jonaskuske commented Dec 14, 2022

Is there a workaround I could use to solve this problem?

Using new URL with a proper file path starting with the package name should work I think, as in new URL('my-module-name/path/to/worker.js', import.meta.url).

Relative paths break (new URL('. /path/...') ) and while my-module-name is resolved to the package directory, everything after does not use Node resolution so you can't do stuff like omitting the file extension there.

@GuskiS
Copy link

GuskiS commented Dec 14, 2022

Thanks, after your comment and doing some digging, I came accross this esbuild plugin: @chialab/esbuild-plugin-meta-url
It solves my issues 👌

@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants