Dynamic public path #334

fabb · 2019-06-12T06:57:10Z

This PR contains a:

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 · 2019-06-12T06:57:12Z

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.

alexander-akait

It is breaking change and invalid usage.

Why breaking change?

Other developers can use __webpack_public_path__ in source code for rewriting public path so they get __webpack_public_path__ + __webpack_public_path__ + publicPathToAsset and it is break their apps.

Why it is invalid and how valid?

https://webpack.js.org/guides/public-path/#on-the-fly

Other solution?

Yes, use hook to rewrite path in webpack. Unfortunately it is break very many apps and we can't merge this, also webpack@5 will be has built-in asset loader (i.e. file-loader) and you need to use hook for changing this. So i strongly recommend rewrite you plugin on using hook(s). Thanks

fabb · 2019-06-13T12:40:50Z

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

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.
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.
The change of this PR is purely additional. If prefixPublicPathWithWebpackPublicPath is not explicitly set, clients will get the old behavior.

alexander-akait · 2019-06-13T12:43:34Z

hm, why don't use publicPath as Function?

fabb · 2019-06-13T12:45:19Z

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 · 2019-06-13T12:50:45Z

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 · 2019-06-13T12:53:16Z

oh, looks i see your problem, no need link

fabb · 2019-06-13T12:53:37Z

Ok thx

alexander-akait · 2019-06-13T12:55:11Z

Let's keep open

alexander-akait · 2019-06-13T13:00:50Z

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

file-loader/src/index.js

Line 35 in 903aa75

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

we don't use JSON.stringify here, so potentially break code
file-loader/src/index.js

Line 37 in 903aa75

publicPath = `${

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 · 2019-06-13T13:01:06Z

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 · 2019-06-13T13:01:59Z

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

fabb · 2019-06-13T13:22:04Z

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.

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 · 2019-06-13T13:28:36Z

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

file-loader/src/index.js

Line 34 in 903aa75

if (typeof options.publicPath === 'function') {

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 · 2019-06-13T14:24:51Z

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 · 2019-06-13T14:37:59Z

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

alexander-akait · 2019-07-18T11:00:26Z

/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 · 2019-07-18T14:27:36Z

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 · 2019-07-18T14:45:40Z

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

fabb · 2019-07-18T15:03:22Z

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 · 2019-07-18T15:09:13Z

Feel free to ping me

fabb · 2019-07-19T06:52:16Z

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

alexander-akait

Need add doc about this option and add description to schema

Good work, thanks!

test/publicPath-option.test.js

fabb · 2019-07-20T06:27:49Z

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

alexander-akait

Looks good

alexander-akait · 2019-07-22T10:27:14Z

@fabb let's rebase

when the option is active, and a publicPath is set, it will prefix the provided publicPath with the dynamic __webpack_public_path__

added unit tests for option "prefixPublicPathWithWebpackPublicPath"

removed prefixPublicPathWithWebpackPublicPath and introduced postTransformPublicPath option instead

fabb · 2019-07-22T10:55:36Z

done!

alexander-akait · 2019-07-22T11:05:51Z

Good work!

alexander-akait · 2019-07-22T11:06:17Z

README.md

+  },
+};
+```
+


Oh, i forgot, let's add Example use case for this

i pushed an example, is this what you mean?

oh, i see, just think about additional section in Example in README

ups, i forgot to push the commit i was talking about 😅
i meant this one: 460127d

alexander-akait · 2019-07-23T11:44:38Z

README.md

@@ -603,6 +603,72 @@ Result:
 path/to/file.png?e43b20c069c4a01867c31e98cbce33c9
 ```

+---


Let's use heading here instead ---, something like ### Dynamic public path with __webpack_public_path__ (maybe better name for heading)

i added a little paragraph for explaining the use case. makes sense?

fabb · 2019-08-03T20:18:55Z

@evilebottnawi can we merge the PR?

alexander-akait · 2019-08-05T08:50:22Z

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

prontiol · 2020-03-02T21:51:08Z

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 · 2020-03-03T10:36:13Z

@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

alexander-akait suggested changes Jun 13, 2019

View reviewed changes

fabb closed this Jun 13, 2019

alexander-akait reopened this Jun 13, 2019

joeldenning mentioned this pull request Jun 25, 2019

__webpack_public_path__ and url() webpack-contrib/css-loader#155

Closed

alexander-akait suggested changes Jul 19, 2019

View reviewed changes

test/publicPath-option.test.js Outdated Show resolved Hide resolved

alexander-akait previously approved these changes Jul 22, 2019

View reviewed changes

fabb dismissed alexander-akait’s stale review via 0c84e34 July 22, 2019 10:47

fabb force-pushed the dynamic_public_path branch from a38391f to 0c84e34 Compare July 22, 2019 10:47

fabb added 5 commits July 22, 2019 12:50

feat: added option "prefixPublicPathWithWebpackPublicPath"

fa6f15b

when the option is active, and a publicPath is set, it will prefix the provided publicPath with the dynamic __webpack_public_path__

feat: added unit tests

e776945

added unit tests for option "prefixPublicPathWithWebpackPublicPath"

feat: introduced "postTransformPublicPath" option

4f990d7

removed prefixPublicPathWithWebpackPublicPath and introduced postTransformPublicPath option instead

feat: moved postTransformPublicPath tests to own file

584f7d9

feat: added documentation for the postTransformPublicPath option

64f2eb6

fabb force-pushed the dynamic_public_path branch from 0c84e34 to 64f2eb6 Compare July 22, 2019 10:55

alexander-akait previously approved these changes Jul 22, 2019

View reviewed changes

alexander-akait reviewed Jul 22, 2019

View reviewed changes

feat: added example for the postTransformPublicPath option

460127d

fabb dismissed alexander-akait’s stale review via 460127d July 23, 2019 11:41

alexander-akait reviewed Jul 23, 2019

View reviewed changes

feat: added some description for the postTransformPublicPath example

1bc97dc

alexander-akait approved these changes Aug 6, 2019

View reviewed changes

alexander-akait merged commit c136f44 into webpack-contrib:master Aug 6, 2019

snyk-bot mentioned this pull request Mar 7, 2020

[Snyk] Upgrade file-loader from 0.9.0 to 5.0.2 kogg/kogg.github.io#6

Open

snyk-bot mentioned this pull request Mar 20, 2020

[Snyk] Upgrade file-loader from 1.1.5 to 5.1.0 lwojcik/sc2pte-panel-frontend#82

Closed

snyk-bot mentioned this pull request Apr 9, 2020

[Snyk] Upgrade file-loader from 1.1.5 to 6.0.0 lwojcik/sc2pte-panel-frontend#108

Closed

Dynamic public path #334

Dynamic public path #334

Conversation

fabb commented Jun 12, 2019 • edited

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Jun 12, 2019 • edited

alexander-akait left a comment • edited

Choose a reason for hiding this comment

fabb commented Jun 13, 2019

alexander-akait commented Jun 13, 2019

fabb commented Jun 13, 2019

alexander-akait commented Jun 13, 2019

alexander-akait commented Jun 13, 2019

fabb commented Jun 13, 2019

alexander-akait commented Jun 13, 2019

alexander-akait commented Jun 13, 2019 • edited

fabb commented Jun 13, 2019

alexander-akait commented Jun 13, 2019

fabb commented Jun 13, 2019 • edited

alexander-akait commented Jun 13, 2019 • edited

fabb commented Jun 13, 2019

alexander-akait commented Jun 13, 2019 • edited

alexander-akait commented Jul 18, 2019

fabb commented Jul 18, 2019

alexander-akait commented Jul 18, 2019

fabb commented Jul 18, 2019

alexander-akait commented Jul 18, 2019

fabb commented Jul 19, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

fabb commented Jul 20, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Jul 22, 2019

fabb commented Jul 22, 2019

alexander-akait commented Jul 22, 2019

alexander-akait Jul 22, 2019

Choose a reason for hiding this comment

fabb Jul 23, 2019

Choose a reason for hiding this comment

alexander-akait Jul 23, 2019

Choose a reason for hiding this comment

fabb Jul 23, 2019

Choose a reason for hiding this comment

alexander-akait Jul 23, 2019

Choose a reason for hiding this comment

fabb Jul 24, 2019

Choose a reason for hiding this comment

fabb commented Aug 3, 2019

alexander-akait commented Aug 5, 2019

prontiol commented Mar 2, 2020

alexander-akait commented Mar 3, 2020

fabb commented Jun 12, 2019 •

edited

jsf-clabot commented Jun 12, 2019 •

edited

alexander-akait left a comment •

edited

alexander-akait commented Jun 13, 2019 •

edited

fabb commented Jun 13, 2019 •

edited

alexander-akait commented Jun 13, 2019 •

edited

alexander-akait commented Jun 13, 2019 •

edited