shellscape / webpack-manifest-plugin

webpack plugin for generating asset manifests
MIT License
1.44k stars 184 forks source link

Support publicPath = “auto” #257

Closed karlvr closed 3 years ago

karlvr commented 3 years ago

This PR contains:

Are tests included?

Breaking Changes?

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Webpack 5 introduces (I think) a default publicPath of "auto". The idea is that things can work out an appropriate publicPath at runtime or based on something else. The documentation for it is https://webpack.js.org/guides/public-path/. In Webpack 5 "auto" is the default.

This PR uses the basePath as the best guess for publicPath if the publicPath is set to "auto". I think that's a good best guess...

shellscape commented 3 years ago

Thanks for the PR. I understand where the intent is coming from, but what this will invariably end with is confusion from the userbase of the same kind, over why we're assigning basePath to publicPath. It's better in my opinion to let the users control that and determine whether or not to set it.

karlvr commented 3 years ago

@shellscape just to clarify, it's the other way around; when publicPath is "auto", we set it to basePath. Does that clarify or just a mistype?

karlvr commented 3 years ago

At the moment, to clarify, if you're using Webpack 5, and don't specify a publicPath in your config at all, the result in the manifest are paths like "autocss/example.css".

shellscape commented 3 years ago

Please use the edit feature on replies to cut down on noise. I understand the implication and what the PR is doing, as well as what the default behavior is (there are a few issues mentioning this available via search). Guessing at what the value should be is "magic" that will lead to the condition of confusion I mentioned.

karlvr commented 3 years ago

Sorry for the noise. Perhaps the plugin should throw an error then, rather than just using the word "auto", to alert the user that extra configuration is required? Or use a default like "", which I think might be what happened under Webpack 4?

shellscape commented 3 years ago

A warning to the console would be more appropriate here. If you did choose to display a warning, I would include a link to the webpack docs for that, or to the migration guide.

karlvr commented 3 years ago

Thanks for your reply! Your plugin is a key part of our workflow on 100s of projects.

A final comment in favour of empty string:

The default publicPath in webpack 4 was empty string. In webpack 5 it's "auto". If it was a goal for this plugin to work in the same way as before with a default publicPath, we should use empty string in place of "auto" to produce the same output.

(As someone who has just completed the migration to 5, I appreciate every plugin and loader that didn't subtly change what it did)

Allowing the word "auto" to appear in our manifest will introduce a word that users may not have encountered, as it's the default rather than an explicit setting.

If someone has explicitly set "auto", I don't think they'll be perplexed or disappointed to see the context-relative path in their manifest. The plugin has automatically chosen a useful and workable path, and it's the same effective publicPath that will be chosen by "auto" in other cases, such as referencing a JavaScript file from an HTML file.

I was wrong to use basePath in place of "auto" at the start of this PR. I now believe that empty string is not just the best but the right substitute.

shellscape commented 3 years ago

The default publicPath in webpack 4 was empty string. In webpack 5 it's "auto". If it was a goal for this plugin to work in the same way as before with a default publicPath, we should use empty string in place of "auto" to produce the same output.

This is why I feel that using any default values for both versions of webpack is incorrect. We're trying to assert some correct default behavior, but it's going to be likely incorrect which ever version we assume is priority. The only reasonable way I'll make a change for webpack v5 here is if the version is detected, auto is used only for webpack v5, and the documentation is updated to reflect the different defaults, linking to the webpack docs for v4 and v5's default values, respectively. It's the only catch-all solution that doesn't shake up one userbase over another.

shellscape commented 3 years ago

@karlvr I'm giving this package some attention this week. Any chance you could make the requested changes soonish? Would make sure the changes get merged and released before I move onto other packages again.

sanemat commented 3 years ago

any update?

shellscape commented 3 years ago

@sanemat if you don't see updates in the PR, there are no updates. Don't be that person who asks that question as a means to ping the author. It's notification noise.

shellscape commented 3 years ago

@karlvr since I haven't received a reply from you on my last reply in July, I'll assume this is abandoned. If you'd like to continue this PR please let me know and we'll reopen.