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 note about changing rootURL when using a custom path #504

Merged
merged 1 commit into from Jan 13, 2017

Conversation

ndbroadbent
Copy link
Contributor

I'm new to ember-cli-rails, and this is the first issue I ran into (see #503)

I'm new to ember-cli-rails, and this is the first issue I ran into (see thoughtbot#503)
@dirtyhenry
Copy link

I ran into this issue. The rootURL in Ember's config file needs a trailing / character or you'll get a warning from Ember. In the example from the README file, the trailing / is missing. Maybe it's worth updating as well.

@ndbroadbent
Copy link
Contributor Author

Oh yeah I ran into the trailing slash issue too. I was pretty confused about that. I just don't understand why it's necessary. It seems like a very easy fix, but the Ember developers decided that a trailing slash is very important? What's the reasoning?

It's also pretty annoying, because you can't force a trailing slash in Rails routes. Rails strips all trailing slashes before it hits your routes, so there's no way to check if it is present.

@seanpdoyle
Copy link
Contributor

Thanks for opening this PR, @ndbroadbent!

Would you mind addressing #504 (comment) in this PR as well?

@nruth
Copy link
Contributor

nruth commented Jan 12, 2017

We use rack-rewrite to handle the trailing / issue.

It's bemusing to strip / off for rails pages but keep them for ember pages, but seems to work:

    #remove trailing slashes
    r301 %r{\A/(?!emberapp1)(?!emberapp2)(.*)/\Z}, '/$1'

    # otherwise ember crashes with root-url not containing /
    r302 '/emberapp1', '/emberapp1/'
    r302 '/emberapp2', '/emberapp2/'

@seanpdoyle seanpdoyle merged commit 6764107 into thoughtbot:master Jan 13, 2017
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.

None yet

4 participants