-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[optimizer] Use relative paths instead of absolute publicPath #10854
Conversation
5539969
to
8f4e1ca
Compare
jenkins, test this |
This makes me nervous because we experienced some pretty tough issues with base path support and webpack. Do you have an example that verifies that this will work? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - tested under dev and with nginx proxy
@spalger I'm similarly nervous, but I'm not sure of any decent alternatives. We could inline these files or load them elsewhere, but there's a size cost or we would be limiting ourselves to not using url(). And we would have to find a way to deal with stylesheets in node modules. |
My nginx config:
|
I'm more curious what files are impacted by webpack's |
@spalger Reread the docs, my understanding is any files loaded via webpack beyond the entry point. File loader, require.ensure, img src |
Found an example that this breaks, the images in the URL field formatter example |
@spalger, those are already broken with a base-path. I have a fix I will open a PR for. |
I created a test plugin that uses chunking and it fails because webpack tries to load files from the root of the url. I was able to get this working with these changes though: https://gist.github.com/anonymous/10fcda4f301cf8420aa94ec65559ca04 The |
@spalger In the long term, we need this change in a fundamental sense. The new build system won't allow rewriting of paths in bundles like the current behavior relies on. |
I agree, but in the future webpack bundles will still need a reliable way to load additional files from the build so I think the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ I like the consistency that |
@spalger stylesheets won't use the base tag, so the paths in those will be broken. Are you saying use base without the publicPath? |
@jbudz in what case won't stylesheets use the base tag? Any relative url should use it, maybe the urls are not relative? Maybe they could be made relative? |
Sorry I could have worded that better, I mean paths inside stylesheets. My understanding is elements in the document will use it, but styles like |
When I get a chance I'll look into configuring the publicPath at runtime for the require.ensure case with |
Maybe progress with the dynamic public path. ui framework svgs aren't loading but it looks like everything else is. This is getting messy. |
I'm going to close this one for now. I haven't had any time to work on it. |
Currently, when creating bundles with webpack we use absolute urls to locate static assets in stylesheets. When the base path changes, these bundles have to be regenerated. This forces an optimization step, which short term takes time, and long term gets in the way of us not shipping webpack.
This switches to using relative paths for assets.
An example in optimize/kibana.style.css, before:
After:
Closes #10724