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

Shutdown the pretender server on destroyApp. #281

Merged
merged 1 commit into from
Dec 15, 2015

Conversation

blimmer
Copy link
Contributor

@blimmer blimmer commented Sep 20, 2015

Ember-CLI 1.13.9 and beyond will have a destroy-app helper that we can shutdown the server in.
This patch automatically inserts this behavior, or shows a message if the user doesn't have a destroy-app helper explaining how to fix the problem.
Fixes #226.

@blimmer
Copy link
Contributor Author

blimmer commented Sep 20, 2015

My PR for the destroy-app helper is here

'to see how to fix the problem.' + EOL +
'******************************************************' + EOL
)
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make sure that people saw this. Without the callout *s it was easy to miss. This is how it looked on my machine:

screen shot 2015-09-20 at 3 43 10 pm

@samselikoff
Copy link
Collaborator

Wow man, you are seriously awesome.

Just a thought - if a newbie installs Mirage for the first time and they're hit with that message, it could be a bit disconcerting. Is there anywhere else we could put the message, perhaps as tests are being run, in the console or build output somewhere?

@samselikoff
Copy link
Collaborator

@blimmer what do you think? I'd love to see this merged

@blimmer
Copy link
Contributor Author

blimmer commented Nov 8, 2015

Hey @samselikoff I'm really sorry for the silence on this - I must've missed the messages. I worry a bit about burying the message behind test/build output because when the testem runner comes up, it will hide the message, making it super easy to miss.

They also need to place the file and then re-run the initializer so the server is shut down correctly. Another strategy we could take would be to not message them and place it in documentation and hope that ember-cli has a release soon.

I also rebased this to make it easier to merge. I've got my notification fixed up, so sorry again about the silence.

@blimmer blimmer force-pushed the shutdown-server-on-destroy-app branch from 217cf04 to d7d1f6d Compare November 8, 2015 20:27
@blimmer
Copy link
Contributor Author

blimmer commented Dec 2, 2015

This will also not be an issue for people on up-to-date versions of Ember-CLI now that the associated PR there has been merged and released.

@samselikoff samselikoff added this to the 0.2.0 milestone Dec 13, 2015
@samselikoff
Copy link
Collaborator

Okay, great. So, the message shows once on install, if users are on older versions of Ember CLI, correct? But on newer versions we'll be able to do this transparently?

@blimmer
Copy link
Contributor Author

blimmer commented Dec 14, 2015

That's correct! Once on install for older versions of ember-cli. Newer versions should not show the message.

@samselikoff
Copy link
Collaborator

Ok great! Is this rebased off latest master? It's not letting me merge. (Sorry about the delays, as soon as you rebase I'll merge this in - in fact, you can).

Ember-CLI 1.13.9 and beyond will have a destroy-app helper that we can shutdown the server in.
This patch automatically inserts this behavior, or shows a message if the user doesn't have a destroy-app helper explaining how to fix the problem.
Fixes miragejs#226.
@blimmer blimmer force-pushed the shutdown-server-on-destroy-app branch from d7d1f6d to 2d48d7e Compare December 15, 2015 14:29
@blimmer
Copy link
Contributor Author

blimmer commented Dec 15, 2015

Sounds good - this is now rebased. I also updated the changelog so people know that they need to run the addon blueprint when upgrading.

blimmer pushed a commit that referenced this pull request Dec 15, 2015
Shutdown the pretender server on destroyApp.
@blimmer blimmer merged commit dacecaf into miragejs:master Dec 15, 2015
@blimmer blimmer deleted the shutdown-server-on-destroy-app branch December 15, 2015 14:34
@samselikoff
Copy link
Collaborator

Thank you, great work!
On Tue, Dec 15, 2015 at 9:34 AM Ben Limmer [email protected] wrote:

Merged #281 #281.


Reply to this email directly or view it on GitHub
#281 (comment)
.

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.

docs aren't clear server needs to be shutdown
2 participants