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

webpack tries to resolve new URL('./', import.meta.url) to a module and fails or succeeds (both are wrong) #16878

Open
dhdaines opened this issue Mar 28, 2023 · 15 comments

Comments

@dhdaines
Copy link

dhdaines commented Mar 28, 2023

Bug report

What is the current behavior?

Webpack's resolution of new URL(..., import.meta.url) does something unexpected when the target is a directory, which doesn't correspond to what either the browser or Node will do. For comparison, try this in $BROWSER_OF_YOUR_CHOICE:

<pre id="output">
</pre>
<script type="module">
  const pre = document.getElementById("output");
  pre.innerHTML += new URL("./", import.meta.url).href;
</script>

or in Node, create index.mjs and run it:

console.log(new URL("./", import.meta.url).href);

In both cases you should see something like:

file:///home/dhd/work/webpack-wasm-test/importmeta/

Webpack does ... something else. See below.

If the current behavior is a bug, please provide the steps to reproduce.

Given the index.mjs file above, run:

webpack ./index.mjs

It will fail with an error like this:

ERROR in ./index.mjs 1:12-42
Module not found: Error: Can't resolve './' in '/home/dhd/work/importmeta'
resolve './' in '/home/dhd/work/importmeta'
  Parsed request is a directory
  No description file found in /home/dhd/work/importmeta or above
  No description file found in /home/dhd/work or above
  as directory
    existing directory /home/dhd/work/importmeta
      No description file found in /home/dhd/work/importmeta or above
      using path: /home/dhd/work/importmeta/index
        No description file found in /home/dhd/work/importmeta or above
        no extension
          /home/dhd/work/importmeta/index doesn't exist

(if there is a package.json somewhere in a parent directory it will output some other stuff about that, but it will still fail in the same way)

I can make it "work" if I initialize the current directory as a module:

npm init
npm install -D webpack-cli webpack
touch index.js
npx webpack ./index.mjs

This is not what I want, because it will resolve the new URL above to the module in the current directory, then bundle whatever it can find in that module. All I wanted was a URL that pointed to the current directory! (and a Pepsi)

See the example here: https://github.com/dhdaines/webpack-wasm-test/tree/main/importmeta

What is the expected behavior?

Webpack should just pass new URL('./', import.meta.url) through unchanged. It should not try to resolve a directory as a module.
Ideally it should also pass through unchanged any other instance of new URL(..., import.meta.url) where the target can't be resolved as an asset, since the user might create them after the fact.

Other relevant information:
webpack version: ^5.76.3 => 5.76.3
Node.js version: 14.18.2
Operating System: Linux 5.19 Ubuntu 22.04.2 LTS 22.04.2 LTS (Jammy Jellyfish)

@dhdaines
Copy link
Author

dhdaines commented Mar 28, 2023

Of course, I can also make it "work" if I listen to webpack and create index in the current directory, but the resolved URL in the output code will still be wrong in that case. If I create index.html in dist/ and open it:

<script src="main.js"></script>

Then I get this in the console:

file:///home/dhd/work/importmeta/dist/31d6cfe0d16ae931b73c

@alexander-akait
Copy link
Member

Do you want to ignore only console.log(new URL("./", import.meta.url).href); or all new URL(...) syntax?

Ideally it should also pass through unchanged any other instance of new URL(..., import.meta.url) where the target can't be resolved as an asset, since the user might create them after the fact.

can you clarify, we already do it, or Am I missing something?

@dhdaines
Copy link
Author

Do you want to ignore only console.log(new URL("./", import.meta.url).href); or all new URL(...) syntax?

Ideally it should also pass through unchanged any other instance of new URL(..., import.meta.url) where the target can't be resolved as an asset, since the user might create them after the fact.

can you clarify, we already do it, or Am I missing something?

Hi! As mentioned above, webpack should make new URL(relative_path, import.meta.url) do what you expect it to do, that is, return a URL to the thing referenced by relative_path. Currently, this works with assets that you want to bundle, see: https://webpack.js.org/guides/asset-modules/

The current behaviour of Webpack is fantastic and fabulous in the case where relative_path points to an asset that you want to bundle, i.e. a file relative to the source file containing new URL. Webpack will bundle it, and rewrite the code so that you get a URL to the output asset.

The main problem is when it points to a directory. Webpack will try to resolve it as a module. This is wrong, because new URL is not a module import. Minimally it should just pass it through in this case.

I think it would be helpful if it would also pass through anything else that it is unable to resolve to an asset, but maybe that gets into something beyond my comprehension of how Webpack works...

@dhdaines
Copy link
Author

See specifically https://webpack.js.org/guides/asset-modules/#url-assets for the current (good) behaviour of webpack for files referenced with new URL

@alexander-akait
Copy link
Member

The main problem is when it points to a directory. Webpack will try to resolve it as a module. This is wrong, because new URL is not a module import. Minimally it should just pass it through in this case.

yeah, do you want to send a fix - https://github.com/webpack/webpack/blob/main/lib/dependencies/URLPlugin.js#L52?

@dhdaines
Copy link
Author

The main problem is when it points to a directory. Webpack will try to resolve it as a module. This is wrong, because new URL is not a module import. Minimally it should just pass it through in this case.

yeah, do you want to send a fix - https://github.com/webpack/webpack/blob/main/lib/dependencies/URLPlugin.js#L52?

Sure! Thanks for the pointer to the relevant code, it would have taken me a lot longer to find that ;-) I'll make a PR...

@TheLarkInn
Copy link
Member

@dhdaines let us know if you need any additional guidance. Commenting to keep from getting stale :-)

@dhdaines
Copy link
Author

Thanks! I haven't had the time to look at this in the last couple weeks, but should have some today, I'll let you know!

@alexander-akait
Copy link
Member

alexander-akait commented May 20, 2023

I looked at our code and I am afraid it will be breaking change, anyway I agree with yor and we should do nothing with it (by default as minimum), unfortunately it is not possible to change the logic now (we don't know how many developers can use it and based on our logic it can be used and it will work), anyway we can implement an options for this, i.e. I want:

  • implement an option to allow ignore this
  • set it true by default when you have experiments.futureDefaults
  • set fullySpecified to true for any new URL(...) syntax, i.e. you should always write full URL when you use new URL(...)

Firstly I want to start with 1 and 2 and postpone 3 for webpack 6

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@dhdaines
Copy link
Author

  • implement an option to allow ignore this
  • set it true by default when you have experiments.futureDefaults
  • set fullySpecified to true for any new URL(...) syntax, i.e. you should always write full URL when you use new URL(...)

Firstly I want to start with 1 and 2 and postpone 3 for webpack 6

Ah, thank you - sorry I didn't get any time to look further into this, I was a bit lost in the code. Having it be an experimental option would make sense to me. Commenting to avoid it getting stale as well! I hope I can look at it this week or next!

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

@CooperHash
Copy link

I looked at our code and I am afraid it will be breaking change, anyway I agree with yor and we should do nothing with it (by default as minimum), unfortunately it is not possible to change the logic now (we don't know how many developers can use it and based on our logic it can be used and it will work), anyway we can implement an options for this, i.e. I want:

  • implement an option to allow ignore this
  • set it true by default when you have experiments.futureDefaults
  • set fullySpecified to true for any new URL(...) syntax, i.e. you should always write full URL when you use new URL(...)

Firstly I want to start with 1 and 2 and postpone 3 for webpack 6

i would like to help, but i don't know how to test the code in URLPlugin.js after rewrite

@alexander-akait
Copy link
Member

@alexander-akait
Copy link
Member

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants