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

Fix Css autoupdate for pages with ROOT_URL_PATH_PREFIX set #3111

Closed
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@DanielDornhardt
Contributor

DanielDornhardt commented Nov 17, 2014

Hi, I fixed an issue which broke the CSS autoupdate for me for a few months now. I thought it would be a different problem, some dynamic javascript required to properly init the template or somesuch, but today I decided to check for the source of the problem.

It seems like I found it, it is similar to this one: 0f4f77f which got patched the same way.

The fix works for me, i tried it for projects without a ROOT_URL set as well, please check and, if possible, include it into the core! :)

Best wishes

Daniel

Fix Css autoupdate for pages with ROOT_URL_PATH_PREFIX set
For pages using a ROOT_URL="" setting with a path component (eg.
"myproject.com/beta"), the CSS autoupdate would break the page, because
it would set the autoupdate CSS files' URL to /<longidstring>.css, while it
should have been /beta/<longidstring>.css. Added the required
ROOT_URL_PATH_PREFIX.
@meteor-bot

This comment has been minimized.

meteor-bot commented Nov 17, 2014

@DanielDornhardt: Before we can merge your pull request, you'll need to sign the Meteor Contributor Agreement: https://contribute.meteor.com/

@ghost

This comment has been minimized.

Contributor

ghost commented Jan 9, 2015

+1
I can verify this fix works for me too (with and without the ROOT_URL)

@DanielDornhardt

This comment has been minimized.

Contributor

DanielDornhardt commented Jan 9, 2015

Cool, somebody merge it! I for some reason actually thought this was in Meteor for months now, somehow I didn't see it didn't get merged yet.

@glasser

This comment has been minimized.

Member

glasser commented Jan 9, 2015

Hi. While this looks right intuitively, it's hard to take PRs where there isn't a straightforward way of seeing the issue. Can you add instructions on how to observe the issue? https://github.com/meteor/meteor/wiki/Contributing-to-Meteor#reporting-a-bug-in-meteor

@ghost

This comment has been minimized.

Contributor

ghost commented Jan 10, 2015

@glasser Here's how to recreate:

  1. start a project using ROOT_URL
$ meteor create test
$ cd test/
$ ROOT_URL=http://localhost:3000/app meteor
  1. open http://localhost:3000/app in the browser
  2. edit the .css file
$ echo "html, body {background:red;}" >> test.css
  1. observe
    The CSS doesn't update b/c the this gets injected:
<link rel="stylesheet" ...  href="/df54bf34afd51269e0a6e13ba97aa5d2a9f4a1fb.css">

Wich obviosly produces a 404 b/c it's missing the /app prefix

This pull request simply ensure's that the prefix, whatever it is, get's pre-pended

<link rel="stylesheet" ...  href="/app/df54bf34afd51269e0a6e13ba97aa5d2a9f4a1fb.css">
@DanielDornhardt

This comment has been minimized.

Contributor

DanielDornhardt commented Jan 11, 2015

Thanks for the example, @smallhelm

@glasser

This comment has been minimized.

Member

glasser commented Jan 12, 2015

Thanks, merged!

@glasser glasser closed this in 8339d5e Jan 12, 2015

@mquandalle

This comment has been minimized.

Contributor

mquandalle commented Jan 12, 2015

Wasn't better to use Meteor.absoluteUrl (like what #3450 did) to use the public API and hide implementation details? (not tested)

@ghost

This comment has been minimized.

Contributor

ghost commented Jan 13, 2015

@mquandalle The problem I see with using Meteor.absoluteUrl is that we have to think about https vs http and using 127.0.0.1 vs localhost b/c it's an absolute url, rather than a relative one.

Take a look at #3470 it the same thing but uses Meteor._relativeToSiteRootUrl

@glasser

This comment has been minimized.

Member

glasser commented Jan 14, 2015

Thanks, merged #3470.

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