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 warning when meteor is installing; Update release to 1.3 #45

Closed
wants to merge 11 commits into from
Closed

Add warning when meteor is installing; Update release to 1.3 #45

wants to merge 11 commits into from

Conversation

andreyantipov
Copy link

  • fixed readme create section
  • added verbose warning when meteor is installing
  • updated release to rc3

@sungwoncho
Copy link

This is good. But it seems that every time I run meteor create, CLI says it is installing a meteor 1.3 beta.rc3. I think there is something wrong with the output of meteor show --show-all meteor on my machine.

The below is the output of that command:

  ...
  1.1.11-beta.12             March 1st, 2016       installed
  1.1.11-beta.13             March 9th, 2016
  1.1.11-beta.14             March 9th, 2016
  1.1.11-beta.15             March 9th, 2016
  1.1.11-beta.16             March 10th, 2016      installed
  1.1.11-cordova.1           January 21st, 2016
  1.1.11-cordova.2           January 23rd, 2016
  1.1.11-cordova.3           January 26th, 2016
  1.1.11-cordova.4           February 11th, 2016
  1.1.11-cordova.5           February 12th, 2016
  1.1.11-faster-rebuild.0    December 18th, 2015
  1.1.11-modules.0           December 12th, 2015
  1.1.11-modules.1           December 19th, 2015
  1.1.11-modules.2           December 23rd, 2015
  1.1.11-modules.3           January 9th, 2016     installed
  1.1.11-modules.4           January 11th, 2016    installed
  1.1.11-modules.5           January 23rd, 2016    installed
  1.1.11-modules.6           February 4th, 2016    installed
  1.1.11-modules.7           February 5th, 2016
  1.1.11-modules.8           February 6th, 2016    installed
  1.1.11-rc.0                March 15th, 2016
  1.1.12-rc.1                March 15th, 2016      installed
  1.1.12-rc.2                March 16th, 2016
  1.1.12-rc.3                March 19th, 2016      installed

You can see that instead of 1.3-rc.3, the output says 1.1.12-rc.3. Can you try running this command and see if you have the same problem?

@andreyantipov
Copy link
Author

@sungwoncho hello

You are absolutely right, there is a different versioning format. Same output.
I presume myself to create a meteor bug ticket with that case meteor/meteor/issues/6548 with explanation. But i don't think that will be "fixed" or fixed soon.

So i propose at this time to use both version numerating, one for checking and one for installation, would be that okay?

@sungwoncho
Copy link

Yeah. It'd be confusing but as long as we comment the code, it'd be a good solution fine for the time being.

@andreyantipov
Copy link
Author

Made a small refactoring. Found a cleaner way to get information about which packages is installed.
As answered in meteor/meteor#6548 ticket that you can get information about packages by meteor show METEOR but there is one strange thing that you can't get information about what packages is exactly installed. So, i decided to keep both numeration until there will be better/cleaner way.

Btw. Found another possible way to check which packages is installed using only public numeration. In meteor show METEOR --show-all --ejson and meteor show meteor --show-all --ejson output both releases have same publishedOn date, so we can use it, but it increase time till in finished both requests. So, better to keep like that — both numeration or maybe switch to publishedOn numeration? - need only one request.

  • Updated to latest 1.3 release
  • isInstalled method added
  • Small refactoring
  • Tests?

Two releases.

// Meteor packages
let release = {"system" : "1.1.12", "public" : "1.3"}

Added isInstalled method. Check if meteor package is installed.

// Public and system version of packages must be related with publishedOn date field
// releases can be taken by meteor show --show-all --ejson `meteor or METEOR`
// meteor is system and METEOR is public
let isInstalled = (version) =>
{
   let pkgs = JSON.parse(shelljs.exec(`meteor show meteor --ejson`, {silent: true}).output).versions

   return pkgs.some(pkg => pkg.version === version && pkg.installed)
}

Warn if meteor isn't installed in verbose mode.

// Warn if meteor isn't installed
options.verbose && !isInstalled(release.system) && logger.invoke(`Installing meteor ${release.public}...`);

Also, i'd like make same verbose notice for npm installation because can take a long time to make output more friendly, do i need to create another PR?

  if (process.env.NODE_ENV !== 'test') {
    logger.invoke('after_init');
    shelljs.set('-e');
    let currentPath = shelljs.pwd();
    shelljs.cd(appPath);
    shelljs.exec('npm install', {silent: !options.verbose});
    shelljs.cd(currentPath);
  }

@andreyantipov andreyantipov changed the title Add warning when meteor is installing; Updated release to rc3 Add warning when meteor is installing; Updated release to 1.3 Mar 30, 2016
@andreyantipov andreyantipov changed the title Add warning when meteor is installing; Updated release to 1.3 Add warning when meteor is installing; Update release to 1.3 Mar 30, 2016
@sungwoncho
Copy link

Okay. This solution works. But at first I wasn't sure what the names system and public meant. Also, maintaining corresponding system and public values might become tedious and error prone as more Meteor releases come out. Such task will become quickly out of hand in the future.

What do you think about #47?

@sungwoncho
Copy link

I've merged #47. Let's see how it plays out. Let me know what you think. We can come back to this approach in the future. Thanks for the PR.

@sungwoncho sungwoncho closed this Apr 1, 2016
@andreyantipov
Copy link
Author

What do you think about #47?

Thathat solution is also workable and better, because simply with one nuance — until mantra work on edge version :)

I've merged #47. Let's see how it plays out. Let me know what you think. We can come back to this approach in the future. Thanks for the PR.

Sure.

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.

2 participants