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

[optimizer] Use relative paths instead of absolute publicPath #10854

Closed
wants to merge 3 commits into from

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Mar 22, 2017

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:

.panel-product .panel-heading-elasticsearch {
  background-image: url(/my_base_path/bundles/6a80accb153b8f8177a67951b7fc4cc2.svg);
}

After:

.panel-product .panel-heading-elasticsearch {
  background-image: url(6a80accb153b8f8177a67951b7fc4cc2.svg);
}

Closes #10724

@jbudz
Copy link
Member Author

jbudz commented Mar 23, 2017

jenkins, test this

@spalger
Copy link
Contributor

spalger commented Mar 24, 2017

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?

Copy link
Contributor

@tylersmalley tylersmalley left a 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

@jbudz
Copy link
Member Author

jbudz commented Mar 24, 2017

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

@jbudz
Copy link
Member Author

jbudz commented Mar 24, 2017

My nginx config:

http {
  server {
    listen 5620;
    location /foo/ {
      proxy_pass http://localhost:5601/;
    }
  }
}

@spalger
Copy link
Contributor

spalger commented Mar 24, 2017

I'm more curious what files are impacted by webpack's publicPath option so we can make sure that they are working with the new setting

@tbragin tbragin added the Team:Operations Team label for Operations Team label Mar 27, 2017
@jbudz
Copy link
Member Author

jbudz commented Mar 28, 2017

@spalger Reread the docs, my understanding is any files loaded via webpack beyond the entry point. File loader, require.ensure, img src

@jbudz jbudz removed the v5.4.0 label Mar 31, 2017
@spalger
Copy link
Contributor

spalger commented Apr 3, 2017

Found an example that this breaks, the images in the URL field formatter example

@tylersmalley
Copy link
Contributor

@spalger, those are already broken with a base-path. I have a fix I will open a PR for.

@tylersmalley
Copy link
Contributor

@spalger @jbudz - created issue to resolve the URL field formatter example: #11082

@spalger
Copy link
Contributor

spalger commented Apr 7, 2017

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 <base> tag isn't one I've seen used often, but it allows you to tell the browser how to resolve relative urls, which means that webpack can use ./bundles/ as it's public directory and it will resolve fine regardless of the url that loads the bundle or the basePath. This changes the way that all relative urls are resolved though, which means that other things break, but it's an option if we really want this.

@epixa
Copy link
Contributor

epixa commented Apr 7, 2017

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

@spalger
Copy link
Contributor

spalger commented Apr 7, 2017

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 <base> tag solution is necessary here.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

@epixa
Copy link
Contributor

epixa commented Apr 7, 2017

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 tag solution is necessary here.

++ I like the consistency that <base> provides as well. It's something kibana core can change as necessary to accommodate configurations like basePath and the value itself is something that plugins don't need to consider.

@jbudz
Copy link
Member Author

jbudz commented Apr 11, 2017

@spalger stylesheets won't use the base tag, so the paths in those will be broken. Are you saying use base without the publicPath?

@spalger
Copy link
Contributor

spalger commented Apr 11, 2017

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

@jbudz
Copy link
Member Author

jbudz commented Apr 11, 2017

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 background-image: url() won't . Those paths are based on the publicPath. With the patch above things like the kibana logo are missing the base path.

@jbudz
Copy link
Member Author

jbudz commented Apr 11, 2017

When I get a chance I'll look into configuring the publicPath at runtime for the require.ensure case with window.__webpack_public_path__. That may give us the relative paths in bundled stylesheets and the path we want for js.

@jbudz
Copy link
Member Author

jbudz commented Apr 18, 2017

Maybe progress with the dynamic public path. ui framework svgs aren't loading but it looks like everything else is. This is getting messy.

@jbudz
Copy link
Member Author

jbudz commented Apr 18, 2017

kbnHrefs are off too,
image

@jbudz
Copy link
Member Author

jbudz commented May 8, 2017

I'm going to close this one for now. I haven't had any time to work on it.

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

Successfully merging this pull request may close these issues.

None yet

6 participants