feat(server): Add warning if using an unsupported version of Node #1657

hyperreality · 2016-11-29T19:58:08Z

As suggested by @codydaig at #1655. A lot of the issues here recently have been raised by people simply running the wrong versions of Node, and while the majority are errors during the npm install step, sometimes the user will inadvertently manage to run MEAN with the wrong Node version and encounter weird errors... hopefully this will prevent that.

hyperreality · 2016-11-29T20:05:16Z

I've also taken the liberty to add an extra condition that disables the warning about an empty config.domain, unless in production environment mode. It is offputting for a new user to be presented with an "important warning" like that when they are running the app for the first few times on their local machine.

simison · 2016-11-29T20:42:40Z

config/lib/app.js

@@ -47,6 +47,7 @@ module.exports.start = function start(callback) {
      console.log(chalk.green('App version:     ' + config.meanjs.version));
      if (config.meanjs['meanjs-version'])
        console.log(chalk.green('MEAN.JS version: ' + config.meanjs['meanjs-version']));
+      console.log(chalk.green('Node version:    ' + process.version.substr(1)));


@hyperreality please ping me when/if this PR goes in and I'll remove similar line from my PR. Cheers!

I do prefer the location of this line in Mikael's PR. Seems more apropos for it to be grouped with the app, server, and db settings display.

@simison will do! Good point @mleanos

simison · 2016-11-29T20:45:00Z

Hmm, are there really a lot of these cases where npm install on unsupported node version passes but the app doesn't run? I'm a bit sceptical. :-)

simison · 2016-11-29T20:45:30Z

BTW a fine implementation of the check otherwise!

simison · 2016-11-29T20:46:23Z

config/config.js

 */
 var validateDomainIsSet = function (config) {
-  if (!config.domain) {
+  if (!config.domain && process.env.NODE_ENV === 'production') {


A separate PR for this would be more suitable?

+1 on this change in a separate PR. However, I'm not sold on the idea. I understand that it may be alarming to first time users of this framework. However, the byproduct of this change could be more impacting & scary for a user that sees this warning for the first time when they deploy their app to a production environment.

Does it make more sense to leave this warning for all environment's to at least make the user aware of this potential config flaw? As they use the app in their development environment, a user will realize that it doesn't really stop their app from working but it's something they must think about as they approach deployment to production.

My rationale behind the change was the following:

Seeing many people install new software, getting a red error message about configuration post-install is unexpected - the first thought is that something went wrong during the installation process, probably with Node.

By the time users actually deploy, I would expect them to have run the app several times in production mode in a safe environment to check it: providing multiple opportunities to see the message.

Anyway, I can see where you're coming from @mleanos. I think what we really need is just a better and more descriptive message. What do you mean by scary implications?. I believe you might be best placed to write the message ;-). I'll remove the change for now.

I see what you're saying @hyperreality. However, maybe we could just "introduce" them to the idea of the domain setting needing to be set; can we show this message as a warning in non-Production environments, and display as a critical (red) error when in production?

I don't want to over-complicate that matter, but I do think we can address the underlying issue of user's getting confused. My statement of "scary" was more that after some time spent on development, with everything working as expected, and to all of a sudden see a message like that in a production environment (whether local or deployed) could give someone a moment of panic. Either way, we can pick this up in a separate PR.

mleanos · 2016-11-29T22:53:00Z

Hmm, are there really a lot of these cases where npm install on unsupported node version passes but the app doesn't run? I'm a bit sceptical. :-)

I'm wondering this as well. I am feeling the pain a little bit with all the support issues coming in that are related to the Node version; not to mention the issues revolving around the generator :(

I do like the implementation of the check though. So I'm not opposed to this going in, even if it is redundant. I'm more concerned with what the real issues are with running Node 7. Are they easily fixed, or are they more-or-less issues with the packages we're using?

mleanos · 2016-11-29T22:56:56Z

@codydaig I assigned this to you since it's very related to #1655 & has some overlap changes.

hyperreality · 2016-11-30T01:36:34Z

It's not just the npm install situation that wrong Node versions affect. A scenario you might find yourself in is accidentally running MEAN with a different version of Node than you installed it with, if you used nvm to switch Node versions for another project. This can be a source of really unusual bugs until you suddenly realise what you're doing. Although I do admit that realistically, if your version is really old, a module will usually complain cryptically first...

hyperreality · 2016-11-30T01:49:15Z

It turns out that we could probably eliminate this whole class of issues more efficiently, with a "preinstall": "node scripts/checkPrerequisites.js" line in package.json, which would automatically run before npm install. Just testing the water here: how does that sound?

hyperreality · 2016-11-30T02:08:57Z

@mleanos I just tried MEAN with Node 7.2.0 and the only problems I had were with Node-sass, commenting that out of the gulpfile and everything ran smoothly - and with the server-side Mocha tests. Obviously that's after installing everything with a lower version of Node, #1652 had problems with npm install.

mleanos · 2016-11-30T05:47:20Z

@hyperreality I like the idea of the preinstall script. Let's go down that route.

hyperreality · 2016-11-30T10:50:43Z

I just wrote the preinstall script which worked nicely on Node < 4.6.0 but strangely it didn't seem to run on Node 7. Turns out NPM 3 has a problem, actually running preinstall post-install: npm/npm#10379, which has been around for a year now.

So maybe this truly is impossible to achieve.

simison · 2016-11-30T10:54:30Z

How about this?

scripts: {
  "check-prerequisites": "node scripts/checkPrerequisites.js",
  "install": "npm run check-prerequisites && npm install"
}

Edit: nope, that's obviously a loop. :-) A bit sleep here.

hyperreality · 2016-11-30T12:16:17Z

It turns out that in Npm 3.10.8 onwards the preinstall script does run before node_modules is installed, but only after http requests have been made. So it could still be useful.

However, because modules have not been installed at that stage the checkPrequisites file would have to bundle a fair amount of the Semver library code, what do you think about this @simison @mleanos?

simison · 2017-06-15T15:12:29Z

@lirantal @mleanos, do we want this check in? If so i could write some notes about code changes

lirantal · 2017-06-15T16:19:04Z

do be honest I don't think I like this in.
it should be clear by the README and package.json's engines to which node.js versions are supported. adding this boilerplate code seems overhead to me.

simison · 2017-06-15T16:20:52Z

+1 @lirantal would make sense for CMS but mean.js is a bolerplate...

mleanos · 2017-06-15T20:40:44Z

it should be clear by the README and package.json's engines to which node.js versions are supported.

Agreed.

hyperreality added 2 commits November 29, 2016 19:53

Add warning if using an unsupported version of Node

a0b32ba

Only warn about config.domain if in production environment mode

396dcb3

simison reviewed Nov 29, 2016

View reviewed changes

mleanos assigned codydaig Nov 29, 2016

simison mentioned this pull request Dec 6, 2016

npm install failed for node-inspector #1675

Closed

lirantal closed this Jun 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): Add warning if using an unsupported version of Node #1657

feat(server): Add warning if using an unsupported version of Node #1657

hyperreality commented Nov 29, 2016 •

edited

Loading

hyperreality commented Nov 29, 2016 •

edited

Loading

simison Nov 29, 2016 •

edited

Loading

mleanos Nov 29, 2016

hyperreality Nov 30, 2016

simison commented Nov 29, 2016

simison commented Nov 29, 2016

simison Nov 29, 2016

mleanos Nov 29, 2016

hyperreality Nov 30, 2016 •

edited

Loading

mleanos Nov 30, 2016

mleanos commented Nov 29, 2016

mleanos commented Nov 29, 2016

hyperreality commented Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

mleanos commented Nov 30, 2016

hyperreality commented Nov 30, 2016

simison commented Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

simison commented Jun 15, 2017

lirantal commented Jun 15, 2017

simison commented Jun 15, 2017 via email •

edited

Loading

mleanos commented Jun 15, 2017

feat(server): Add warning if using an unsupported version of Node #1657

feat(server): Add warning if using an unsupported version of Node #1657

Conversation

hyperreality commented Nov 29, 2016 • edited Loading

hyperreality commented Nov 29, 2016 • edited Loading

simison Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

mleanos Nov 29, 2016

Choose a reason for hiding this comment

hyperreality Nov 30, 2016

Choose a reason for hiding this comment

simison commented Nov 29, 2016

simison commented Nov 29, 2016

simison Nov 29, 2016

Choose a reason for hiding this comment

mleanos Nov 29, 2016

Choose a reason for hiding this comment

hyperreality Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

mleanos Nov 30, 2016

Choose a reason for hiding this comment

mleanos commented Nov 29, 2016

mleanos commented Nov 29, 2016

hyperreality commented Nov 30, 2016 • edited Loading

hyperreality commented Nov 30, 2016 • edited Loading

hyperreality commented Nov 30, 2016 • edited Loading

mleanos commented Nov 30, 2016

hyperreality commented Nov 30, 2016

simison commented Nov 30, 2016 • edited Loading

hyperreality commented Nov 30, 2016 • edited Loading

simison commented Jun 15, 2017

lirantal commented Jun 15, 2017

simison commented Jun 15, 2017 via email • edited Loading

mleanos commented Jun 15, 2017

hyperreality commented Nov 29, 2016 •

edited

Loading

hyperreality commented Nov 29, 2016 •

edited

Loading

simison Nov 29, 2016 •

edited

Loading

hyperreality Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

simison commented Nov 30, 2016 •

edited

Loading

hyperreality commented Nov 30, 2016 •

edited

Loading

simison commented Jun 15, 2017 via email •

edited

Loading