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

Add publicPathRelativeToSource option #368

Closed

Conversation

karlvr
Copy link
Contributor

@karlvr karlvr commented Mar 27, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Addresses #367 by adding a publicPathRelativeToSource option to dynamically adjust the publicPath passed down to import loaders so that paths in the output CSS will be relative to the source CSS file rather than the context.

The browser treats URLs in CSS files relative to the CSS file, so having URLs be relative to the CSS file is natural in CSS. A common solution is to set publicPath to an absolute path, but that isn't always possible. Another common solution is to set publicPath to ../, which assumes that your CSS file is output into ./css/ (or some other folder, but just one folder) and fails if you output a CSS file more than one folder deep.

(URLs in the CSS that comes into mini-css-extract-plugin are made relative to the context by css-loader, which produces JavaScript where URLs should be relative to the context)

Note that this solution only works if the output CSS is at the same depth relative to the context as the source file.

Breaking Changes

N/A

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Mar 27, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very weird, please create minimum reproducible test repo, i can't understand you structure and what is problem. Don't forget you can use __webpack_public_path__ if you want to change publicPath (https://webpack.js.org/configuration/output/#outputpublicpath) inside code.

@karlvr
Copy link
Contributor Author

karlvr commented Mar 27, 2019

@evilebottnawi I shall put together an example!

I can't use __webpack_public_path__ as I output multiple CSS files, nested at different depths, e.g. css/main.css and css/admin/main.css so unless I use an absolute path in publicPath or __webpack_public_path__ there isn't one-size to fit all.

Example to come...

@karlvr
Copy link
Contributor Author

karlvr commented Mar 28, 2019

mcep-pprts-example.zip

Here is a zip of a very basic project that demonstrates the issue. publicPath is set to ../ as I don't want to / can't use an absolute publicPath.

The project is setup to use my fork with the code in this PR, so you can turn on/off publicPathRelativeToSource in the webpack.config.js.

npm install
npx webpack

Then look at build/css/main.css, this has a reference to the image that will work: ../img/background.png.

Then look at build/css/admin/admin.css, this has a reference that won't work, also ../img/background.png but it needs to be ../../img/background.png.

This PR would made that relative URL ../../img/background.png.

Edit webpack.config.js and uncomment the line with publicPathRelativeToSource and run webpack again and observe the change. Both CSS files now have valid relative paths.

@alexander-akait
Copy link
Member

Please search issue in file loader about this and read why it was removed and can break your code in many cases, it is really unsafe, I try to search other approach to solve your problem, but maybe it is won't fix, other bundlers also don't do this, even you do fork it is solution only for you case in your project and can lead to problem in future

@karlvr
Copy link
Contributor Author

karlvr commented Mar 30, 2019

@evilebottnawi thank you, I looked at file-loader history and I found what I believe was the bug you are referring to: webpack-contrib/file-loader#221

However I don't think this bug is relevant in this case. File-loader is indeed the wrong place for this approach, as it outputs many different file types while this module only outputs one.

My PR creates URLs relative to the CSS file, which is how CSS URLs work. Whereas file-loader results go on to be used in JavaScript, HTML and CSS which have conflicting relative path handling.

In my opinion, every solution that ends up setting a relative publicPath is the wrong approach, as it requires all CSS files at the same depth. So I hope we can work out an appropriate solution and not be left with that!

Are you able to point me to or suggest hiw this approach fails (aside from the failure case I identified regarding output being at a different depth than source)? That would really help me understand the problem that I’m not currently seeing clearly!

(For reference, this is the main source of info I found in file-loader webpack-contrib/file-loader#221 and also relevant webpack-contrib/file-loader#32 webpack-contrib/file-loader#257 webpack-contrib/file-loader#260)

The webpack-contrib/file-loader#221 seems to suggest that if file-loader outputs a resource once its output may be reused by other loaders and therefore be invalid. Is that true? Wouldn’t that be a problem simply by specifying publicPath on this loader?

@alexander-akait
Copy link
Member

I think support publicPath as function should solve you issue and it is more generic solution, what do you think?

@karlvr
Copy link
Contributor Author

karlvr commented Apr 3, 2019

@evilebottnawi it will definitely solve my issue, and I will make a PR for that as it will be nice to have this solvable. I do think that given the interest in relative publicPath that actually having a solution as easy as a boolean configuration option is useful! I just wish the loader could know where its output was going!

@alexander-akait
Copy link
Member

@karlvr feel free to update/create new PR 👍

@karlvr karlvr mentioned this pull request Apr 4, 2019
6 tasks
@alexander-akait
Copy link
Member

Can we close this PR?

@karlvr karlvr closed this Apr 9, 2019
@karlvr
Copy link
Contributor Author

karlvr commented Apr 9, 2019

All good. I've closed it. I'm going to continue to tinker and see if I can get access to the output file path nicely and do the relative paths comprehensively; for myself and I'll try to share again :-)

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

Successfully merging this pull request may close these issues.

3 participants