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

Passing options to tiny-lr (live reload) for HTTPS support #4249

Merged
merged 1 commit into from Jun 9, 2015
Merged

Passing options to tiny-lr (live reload) for HTTPS support #4249

merged 1 commit into from Jun 9, 2015

Conversation

dosco
Copy link

@dosco dosco commented Jun 5, 2015

Ember CLI is currently creating an instance of tiny-lr server without passing an options object which causes the live reload to always start to HTTP mode, with the newly added HTTPS support in express-server we an now pass the key and cert to tiny-lr allowing it to also launch in HTTPS mode.

Currently when ember serve is launched in HTTPS mode live reload will not work due to mixed-content issues in the browser.

@stefanpenner
Copy link
Contributor

Nice. Can you add a test?

@ghost
Copy link

ghost commented Jun 6, 2015

Hah, i was going to submit the same PR. Do you think that live reload should have an option for its own key and cert and then default to options.sslKey, options.sslCert if they aren't set? Or does that not really matter?

@dosco
Copy link
Author

dosco commented Jun 6, 2015

@jrjohnson this is mostly for dev. environments where the certs are for the most part invalid but on the same domain as the app so having a different cert for LR would make it hard for dev's to accept the exception and to make it work in the browser.

@stefanpenner working on the tests.

@ghost
Copy link

ghost commented Jun 6, 2015

ah, i had setup my own local CA for dev, so all my certs are valid. I'm personally happy with this PR as is. :)

@@ -47,7 +57,7 @@ module.exports = Task.extend({
this.expressServer.on('restart', this.didRestart.bind(this));

// Start LiveReload server
return this.listen(options.liveReloadPort)
return this.listen(options)
.then(this.writeBanner.bind(this, options.liveReloadPort))
.catch(this.writeErrorBanner.bind(this, options.liveReloadPort));
Copy link

Choose a reason for hiding this comment

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

should we show that live reload is using ssl in the banner?

Copy link
Contributor

Choose a reason for hiding this comment

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

also yes

@chadhietala
Copy link
Member

@dosco Can you squash your commits? Otherwise looks good to me.

server.listen(port, resolve);
});
server.listen(options.liveReloadPort, resolve);
}.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't see where this is used inside this function (maybe I'm reading the diff wrong)

@stefanpenner
Copy link
Contributor

left a comment, otherwise (including @chadhietala comment) LGTM

@chadhietala
Copy link
Member

I don't think Windows failures are related. Restarting for good measure. Also Coveralls has interesting opinions on math.

@stefanpenner
Copy link
Contributor

Needs rebase but Lgtm

@chadhietala
Copy link
Member

@dosco Can you rebase?

@stefanpenner
Copy link
Contributor

two more comments

chadhietala added a commit that referenced this pull request Jun 9, 2015
Passing options to tiny-lr (live reload) for HTTPS support
@chadhietala chadhietala merged commit a74f35f into ember-cli:master Jun 9, 2015
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

3 participants