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

Display message to user if an ember-cli update is available (and add ember update command) #899

Merged
merged 3 commits into from
Jun 20, 2014

Conversation

andycrum
Copy link
Contributor

@andycrum andycrum commented Jun 5, 2014

This is a first pass at checking for ember-cli updates and displaying a message to the user if an update is available (fixes #335).

TODOs

  • Persistence via .ember-cli
  • Option to disable via .ember-cli
  • ember update command
  • Tests

@jgwhite
Copy link
Contributor

jgwhite commented Jun 5, 2014

Nice!

@rwjblue
Copy link
Member

rwjblue commented Jun 5, 2014

Persistence should be handled by yam (pending in another PR), which adds a .ember-cli configuration file.

@rwjblue
Copy link
Member

rwjblue commented Jun 5, 2014

We should also have a flag to disable the check (think mobile with no network scenarios).

@rwjblue
Copy link
Member

rwjblue commented Jun 5, 2014

👍 - I like it.

@stefanpenner
Copy link
Contributor

I'm a bit concerned about using npm directly for this. As it is often a bit of a slow boot problem.

Can we issue a quick raw http request to its api?

@andycrum
Copy link
Contributor Author

andycrum commented Jun 5, 2014

@stefanpenner yeah... If I get persistence working (after the .ember-cli PR merge), that would cut it down to only needing to use npm once per day (or less). Would using it directly still be a problem then?

@stefanpenner
Copy link
Contributor

@andycrum loading NPM itself is like 400ms -> 500ms regardless of the HTTP request. So as long as we can defer that aswell

@andycrum
Copy link
Contributor Author

Running into some really strange test errors with generate-test.js. At first thought it was just blueprint stuff (.ember-cli was missing in some of the blueprint fixtures, etc.), but still happening. Looking into it.

@jgwhite
Copy link
Contributor

jgwhite commented Jun 12, 2014

@andycrum can you post the error output? Might be something I've seen.

@twokul twokul added this to the v0.1.0 milestone Jun 12, 2014
@andycrum
Copy link
Contributor Author

@stefanpenner @rjackson @jgwhite looks like tests are passing now. would you guys mind reviewing?

@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This is already being done (https://github.com/stefanpenner/ember-cli/blob/master/blueprints/app/files/.ember-cli), is there a reason to have two files and rename?

@andycrum andycrum changed the title [WIP] Display message to user if an ember-cli update is available [WIP] Display message to user if an ember-cli update is available (and add ember update command) Jun 14, 2014
@andycrum
Copy link
Contributor Author

Rebased and fixed a couple bugs. @stefanpenner @rjackson what still needs to be done here?

return new Promise(function(resolve) {
// if the 'checkForUpdates' setting is true, check for an updated ember-cli version
// if environment.settings is undefined, that means there is no .ember-cli file, so check by default
if(commandName !== 'update' && (typeof environment.settings === 'undefined' || environment.settings.checkForUpdates)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OCD: 80 char line limit

@stefanpenner
Copy link
Contributor

some superficial feedback, but this looks good!

@andycrum
Copy link
Contributor Author

@stefanpenner I think I've now addressed most of your notes. Couldn't get Promise.denodeify working where you mentioned, but can look at it in further detail if it's important...

@andycrum andycrum changed the title [WIP] Display message to user if an ember-cli update is available (and add ember update command) Display message to user if an ember-cli update is available (and add ember update command) Jun 19, 2014
…is available

Add ember update command and change update message to suggest running that rather than npm install

Persist checkForUpdates setting and version information via .ember-cli file

Add .ember-cli files to blueprint tests/fixture directories where needed

Wrap checkForUpdates call/command.validateAndRun() in a Promise to prevent async weirdness

Remove unnecessary ember-cli file (already there)

Make an HTTP call to npm when checking for updates rather than loading npm module

Syntax fix and remove stray call in test to create ember-cli file (not needed)

Add test to confirm update command is run

Update the package.json file manually after doing a global install, so
the correct ember-cli version is reflected there

Add a couple unit tests for update command/update task

fix syntax errors

Remove debug statement

Add entry to CHANGELOG
…mise // Use .catch() instead of then(success, failure) syntax // Fix some 80+ char lines
@andycrum
Copy link
Contributor Author

Think I've gotten everything now... got Promise.denodeify working. Anything else keeping this from being merged? @stefanpenner @rjackson

stefanpenner added a commit that referenced this pull request Jun 20, 2014
Display message to user if an ember-cli update is available (and add `ember update` command)
@stefanpenner stefanpenner merged commit 8e76e30 into ember-cli:master Jun 20, 2014
@stefanpenner
Copy link
Contributor

awesome thanks!

@devinrhode2
Copy link

Why isn't ember update mentioned in the site documentation http://www.ember-cli.com/#upgrading?

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.

inform users when a new CLI is available
6 participants