Dynamic public path

[fabb]

This PR contains a:

• [ ] bugfix
• [x] new feature
• [ ] code refactor
• [x] test update
• [ ] typo fix
• [ ] metadata update

Motivation / Use-Case

We use the file-loader in a next.js plugin to automatically generate asset urls by importing the assets. The asset file names will be hashed, which is good for caching indefinitely. The plugin supports next.js' assetPrefix which allows to provide a different cache server host url for static assets. Next.js uses __webpack_public_path__ by default.

The problem is that in our case the assetPrefix is dynamic at runtime, and will be set based on environment variables which are not present at compile time. The plugin-static gets used during compile time though, and will only use the assetPrefix that is based on environment variables present at compile time. The proper way around this is to make use of __webpack_public_path__. The problem with file-loader is that when setting the publicPath, it is not possible to use the __webpack_public_path__. Therefore a new setting is needed which allows to prefix with __webpack_public_path__.

Breaking Changes

No breaking changes.

Additional Info

[jsf-clabot]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

[fabb]

That‘s too bad, and I don‘t completely understand it. Could it be that there is a misunderstanding on what the PR does?

1. With vanilla file-loader, when not setting the publicPath, it will default to __webpack_public_path__ + ${JSON.stringify(outputPath)}, which is basically the same as what‘s possible with this PR.
2. In this PR, __webpack_public_path__ is not set, it is just returned, so it will be used at runtime to build the public path. Exactly this is necessary so clients can set it according to the documentation you mentioned, and get a path depending on runtime environment variables.
3. The change of this PR is purely additional. If prefixPublicPathWithWebpackPublicPath is not explicitly set, clients will get the old behavior.

[alexander-akait]

hm, why don't use publicPath as Function?

[fabb]

That is not possible, because the function is executed at buildtime, but I need to modify the publicPath at runtime depending on runtime environment variables that cannot be present at buildtime.

[alexander-akait]

Please provide plugin/app/reproducible repo with problematic and describe what you want to solve in README.md.

As i said above webpack@5 will have built-in file-loader so you solution can't work in next webpack version, we should search way how to solve this in other way.

[alexander-akait]

oh, looks i see your problem, no need link

[fabb]

Ok thx

[alexander-akait]

Let's keep open

[alexander-akait]

I think we need do breaking change again to solve this, publicPath option is broken:

1. https://github.com/webpack-contrib/file-loader/blob/903aa7544f7c25d524e25a7dc171e9d4d6f80980/src/index.js#L35 we don't use JSON.stringify here, so potentially break code
2. https://github.com/webpack-contrib/file-loader/blob/903aa7544f7c25d524e25a7dc171e9d4d6f80980/src/index.js#L37 we don't add __webpack_public_path__ here (i think we should do this)

Good catch, anyway publicPath should works as workaround for you:

publicPath(url) {
  return '__webpack_public_path__' + url;
}

[fabb]

Thanks for taking the time to understand the problem!

Until I can switch to webpack 5 it‘s fine for me to use my forked package.

[alexander-akait]

I think we should correct this behavior and migrate all tests to webpack@5 to ensure all works es expected

[fabb]

I think we need do breaking change again to solve this, publicPath option is broken:

1. publicPath = options.publicPath(url, this.resourcePath, context); 

   we don't use JSON.stringify here, so potentially break code.

Not true because there is not return. In line 44 it is stringified.

2. publicPath = `${ 

   we don't add webpack_public_path here (i think we should do this)

Also not true because there is no return. Line 46 will be executed in either way.

Good catch, anyway publicPath should works as workaround for you:

publicPath(url) {
  return '__webpack_public_path__' + url;
}

No, this does not work, because of the JSON.stringify in line 44. This will add quote around the whole string like this: "__webpack_public_path__ + passed_url" which cannot work.

[alexander-akait]

No, this does not work, because of the JSON.stringify in line 44. This will add quote around the whole string like this: "webpack_public_path + passed_url" which cannot work.

Not true because there is not return. In line 44 it is stringified.

Do you try this? We don't add anything when publicPath is function https://github.com/webpack-contrib/file-loader/blob/903aa7544f7c25d524e25a7dc171e9d4d6f80980/src/index.js#L34

Also not true because there is no return. Line 46 will be executed in either way.

It is hack to solve problem what i describe above and we need fix it as bug with major release

[fabb]

Do you try this? We don't add anything when publicPath is function

Not true. Line 44 is also executed in this case afterwards, because there is no return in line 35. And yes, I tried it 😉

[alexander-akait]

Oh, yes sorry, anyway you catch the bug and we fix it in next release (workaround: use forked repo right now)

[alexander-akait]

/cc @fabb sorry for delay, let's use better name (ideas?), also let's do this as String|Function (default string is __webpack_public_path__)

[fabb]

Sorry, no better ideas for naming 🙈

String|Function is a good idea, i guess you mean for the publicPath option? Then we don‘t need to find a better name for prefixPublicPathWithWebpackPublicPath, because we don‘t need it at all, right?

[alexander-akait]

Maybe just postTransformPublicPath because you can add any own global variable before publicPath not only __webpack_public_path__ when you use Function?

[fabb]

Ah, good idea. I just saw that publicPath can already be a function, but the result still gets stringified, so for backwards compatibility, a new option is needed anyways (or overload the return value of the publicPath function, but that gets too complicated. Ok will change the PR soon.

[alexander-akait]

Feel free to ping me

[fabb]

@evilebottnawi I've implemented the change. I fear it might be a bit confusing for users on how to use the option correctly. Please take a look at the unit tests and tell me what you think.

[fabb]

ok done! any additional remarks? should we add resourcePath and context as parameters as well for flexibility?

[alexander-akait]

@fabb let's rebase

[fabb]

done!

[alexander-akait]

Good work!

[fabb]

@evilebottnawi can we merge the PR?

[alexander-akait]

@fabb yep, at this week we merge and release, in my todo

[prontiol]

I remember you was against it, @evilebottnawi?

Quotes are yours:

Because it is fix only logic for file-loader, but other loader also can emit assets, i think we need solve this on webpack side, so i request minimum reproducible test repo

Does this PR fix this on webpack side? Where's the "minimum reproducible test repo"?

We don't want add new option to file-loader, because in webpack@5 it was built-in loader, so we need solve this problem on webpack level to avoid same problems in future for other developers

You refused to merge my PR even if it was not adding any new options, but merged this one with an additional option?

[alexander-akait]

@prontiol A similar feature was implemented on webpack side and several people requested a similar solution for file-loader.

Sometimes I'm mistaken, I should have heard you better, sometimes it’s difficult because of the influx of many ideas, most of which look terrible, sorry