webpack icon indicating copy to clipboard operation
webpack copied to clipboard

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

Open dhdaines opened this issue 2 years ago • 18 comments
trafficstars

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 avatar Mar 28 '23 14:03 dhdaines

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

dhdaines avatar Mar 28 '23 14:03 dhdaines

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?

alexander-akait avatar Mar 28 '23 14:03 alexander-akait

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 avatar Mar 28 '23 15:03 dhdaines

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

dhdaines avatar Mar 28 '23 15:03 dhdaines

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?

alexander-akait avatar Mar 28 '23 15:03 alexander-akait

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...

dhdaines avatar Mar 28 '23 15:03 dhdaines

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

TheLarkInn avatar Apr 14 '23 18:04 TheLarkInn

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!

dhdaines avatar Apr 21 '23 13:04 dhdaines

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

alexander-akait avatar May 20 '23 22:05 alexander-akait

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.

webpack-bot avatar Aug 20 '23 14:08 webpack-bot

  • 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!

dhdaines avatar Aug 23 '23 14:08 dhdaines

Issue was closed because of inactivity.

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

webpack-bot avatar Dec 08 '23 08:12 webpack-bot

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

CooperHash avatar Feb 25 '24 11:02 CooperHash

Just put config case here https://github.com/webpack/webpack/tree/main/test/configCases

alexander-akait avatar Feb 27 '24 12:02 alexander-akait

bump

alexander-akait avatar Jun 19 '24 13:06 alexander-akait

I want to work on this issues please assign me this issue.

omkumar312 avatar Aug 11 '24 18:08 omkumar312

Just replace new URL('./', import.meta.url) with new URL('./', Object(import.meta).url) if you don't want webpack to touch it.

seanmorris avatar Sep 22 '24 18:09 seanmorris

Just replace new URL('./', import.meta.url) with new URL('./', Object(import.meta).url) if you don't want webpack to touch it.

Oh! Indeed, that ought to work, thanks!

dhdaines avatar Sep 23 '24 14:09 dhdaines

Just replace new URL('./', import.meta.url) with new URL('./', Object(import.meta).url) if you don't want webpack to touch it.

Not just. it won't work. It transpile to new URL("./",Object({}).url)

b2whats avatar Jan 11 '25 16:01 b2whats

If you do no want any new URL() modification, untap what the URLPlugin has hooked.

class RemoveURLPlugin {
  apply(compiler) {
    compiler.hooks.afterResolvers.tap("RemoveURLPlugin", (resolveOptions) => {
      compiler.hooks.compilation.taps = compiler.hooks.compilation.taps.filter(
        (tap) => {
          const isUrlPlugin = tap.name === 'URLPlugin';
          if (isUrlPlugin) {
            console.log('Removing URL plugin');
          }

          return !isUrlPlugin;
        }
      );
      return resolveOptions;
    })
  }
}

and then

plugins: [
   new RemoveURLPlugin()
]

frederikbosch avatar Feb 13 '25 16:02 frederikbosch

PR welcome to fix it

alexander-akait avatar Feb 13 '25 17:02 alexander-akait

PR welcome to fix it

Could we add one leading ignore comment to opt-out url parsing [just like what rolldown does]?(https://github.com/rolldown/rolldown/issues/2745)

hai-x avatar Feb 14 '25 06:02 hai-x

@hai-x We already have it https://webpack.js.org/configuration/module/#moduleparserjavascripturl 😄 I just think we should not handle new URL('.', import.meta.url) and new URL('./', import.meta.url), mostly they used to resolve current URL, even we will implement bundling JS files using new URL(...) (technically it is possible, but in this case it will be necessary to generate additional runtime), I still think it is very rare and by default we should not doing it, maybe later when we will implement double bundling like I describe above (and under the option)

alexander-akait avatar Feb 14 '25 16:02 alexander-akait