-
-
Notifications
You must be signed in to change notification settings - Fork 53
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 overriding baseURL #28
Conversation
This looks like a good refactor, but you mentioned that it only partially fixes things. What are the remaining issues? |
Well, I think that this addon probably shouldn't use This PR only allows to override it, which didn't fully work before :) My primary goal was to remove |
Hi, just a friendly ping. This commit is just a fix, so I think it can be easily merged and released. The idea of not using |
Ya, this change looks good, but we need to figure out the root issue (pun intended). |
For // maintaining `baseURL` for backwards compatibility. See: http://emberjs.com/blog/2016/04/28/baseURL.html
var baseURL = options.liveReloadBaseUrl || options.rootURL || options.baseURL; |
@@ -28,13 +28,14 @@ module.exports = { | |||
var self = this; | |||
var app = config.app; | |||
var options = config.options; | |||
var baseURL = options.liveReloadBaseUrl || options.baseURL; |
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.
Can you switch this to options.liveReloadBaseUrl || options.rootURL || options.baseURL
?
Thanks for chiming in @nathanhammond. One small tweak and we should be good here... |
Thanks for taking time to look into it. I've updated the PR according to your suggestions. There are 3 commit now, each clearly showing it's purpose. |
Thank you! |
Published |
Thanks! |
Partially fixes #27 by allowing to override baseURL, which is being removed from ember-cli 2.7 (currently in beta).