Add .nvmrc

[Zidail]

• I've checked that this isn't a new swag opportunity proposal.
• I've checked that this isn't a duplicate pull request.

What Changed

• Added .nvmrc file with node version set at v10.17.0
• Remove Node version from TravisCI config so it reads from .nvmrc
• Remove Node version from Netlify config so it reads from .nvmrc

Why?

Just like how we normally lock down which versions of libraries, frameworks or languages we use, we should also lock down the node version as well. This prevents contributors from having "Works on others but not on mine" moments and reduces side-effects of compiling packages with different versions of node. This also helps to make sure that we can get contributors to use the same version of node as what's on the CI.

Notes

v10.17.0 was chose as the default node version because it's the latest version of node specified in TravisCI and Netlify configs.

Edit Updates

Thu Oct 24 05:02:01 UTC 2019

• Added note on why specific version was chosen.
• Updated What's changed to reflect updates to TravisCI and Netlify configs.

[aslafy-z]

Looks good!

Could you drop:

• build.environment section of netlify.toml as netlify rely on it but also supports .nvmrc. See https://www.netlify.com/docs/build-settings/ .
• node_js section of .travis.yml as it's also supported by travis. See https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#specifying-nodejs-versions-using-nvmrc

I'll have to support it within the docker PR also #76.

[vikaspotluri123]

We should check Github Actions as well..

As a side note, can you bump it to the 12.13.x line?

[Zidail]

@aslafy-z Sure thing. I'll update appropriately.

@vikaspotluri123 If we bump the line, I'd recommend bumping our CircleCI config to match as well. Development made with 12 may not be backwards compatible.

[vikaspotluri123]

We don't use CircleCI, which file are you referencing?

[Zidail]

@vikaspotluri123 Sorry, not Circle, I meant TravisCI. Specifically:

https://github.com/swapagarwal/swag-for-dev/blob/master/.travis.yml#L4

I would suggest making the changes that @aslafy-z mentioned first, then checking to see if everything is stable, then bumping up the version of node in a later PR.

[vikaspotluri123]

Oh, gotcha 👍 Let us know if you need something done 😄

[Zidail]

@aslafy-z Updated the PR for the changes requested.
@vikaspotluri123 Thanks! I'll create a new PR for bumping the node version once this PR gets approved and merged.

[vikaspotluri123]

That can be done in this PR; since CI runs on every commit, we should be able to see that everything works

[Zidail]

@vikaspotluri123 Normally I try to keep each PR to a separate topic, so one for adding .nvmrc and one for updating project node version, mainly for transparency sake and for ease of reverting. Let's see what happens when I bump up the node version though.

Looks like Travis failed due to unrelated tests.
Netlify also failed, but I can't debug that since the link just leads me to a blue screen :/

What would you like me to do in this case @vikaspotluri123 ? Revert the update to node version or rerun the processes and see if everything works (although I don't have access to re-run anything)?

[aslafy-z]

I guess you can drop the full section, including version of NPM.

[aslafy-z]

Looks good! I'll go for your proposal (#457 (comment)) since bumping the version makes the build fail. @vikaspotluri123 you confirm?

[vikaspotluri123]

The Netlify build does break, but it seems to break because NPM is out of date 🤔

[aslafy-z]

@vikaspotluri123 Ok, so I guess, applying my comment should do the trick. @Zidail Can you fix it? Thanks

[Zidail]

@vikaspotluri123 @aslafy-z I've updated the Netlify config like you suggested but it's still breaking mainly with the Sharp module.
Travis still breaks on unrelated tests.
Any other ideas?

[vikaspotluri123]

It's not breaking any more 😏 I ran the netlify build with no cache which fixed the issue 🚀

[Zidail]

@vikaspotluri123 Sweeet. Can you run the TravisCI again? The tests are failing on validating promo offers.

[aslafy-z]

I don't mind about the dataset tests, Microsoft website works in here.
Might be related to nodejs/node#27711 (comment)

It's ready to merge for me, @vikaspotluri123 can you confirm?

Thank you @Zidail!

[vikaspotluri123]

Fine with me 🚀

[aslafy-z]

Thank you @Zidail! 🚀

[KiranAkadas]

Hello
I found out that after merging this commit.
There seems to be a constant CI Error for the Microsoft reference due to which #468 isn't getting merged since almost a month.