New: Check node version based on package.json #824
Conversation
stramel
commented
Jul 15, 2017
•
stramel
commented
- CHANGELOG.md has been updated
src/polymer-cli.ts
Outdated
| @@ -12,6 +12,9 @@ | |||
| * http://polymer.github.io/PATENTS.txt | |||
| */ | |||
|
|
|||
| // Bail out if invalid node version | |||
| require('please-upgrade-node')(require('../package.json')); | |||
There was a problem hiding this comment.
We already have similar logic here: https://github.com/Polymer/polymer-cli/blob/master/bin/polymer.js#L27
Also, I'd be hesitant to put statements before import declarations, as that's not actually spec-compliant.
There was a problem hiding this comment.
Doh! I definitely didn't look here.
I put this in the wrong place apparently. The require before import is to ensure that older versions that don't support import. By placing it in bin/polymer.js it is technically before all the other imports.
I will modify this to do the bin/polymer.js
stramel
force-pushed
the
ms/node-version-check
branch
from
03f5674
to
bfabacf
Compare
bin/polymer.js
Outdated
| console.log( | ||
| 'Polymer CLI requires at least Node v6. ' + | ||
| 'Polymer CLI requires at least Node v' + verion.replace(/[^\d\.]*/, '') + '. ' |
bin/polymer.js
Outdated
| console.log( | ||
| 'Polymer CLI requires at least Node v6. ' + | ||
| 'Polymer CLI requires at least Node v' + verion.replace(/[^\d\.]*/, '') + '. ' |
There was a problem hiding this comment.
Missing + at the end of the line.
bin/polymer.js
Outdated
| console.log( | ||
| 'Polymer CLI requires at least Node v6. ' + | ||
| 'Polymer CLI requires at least Node v' + verion.replace(/[^\d\.]*/, '') + '. ' |
There was a problem hiding this comment.
Can you store the version.replace result in a variable and comment what it's doing?
stramel
changed the title
CHANGELOG.md
Outdated
| @@ -1,6 +1,7 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased | |||
| - Drives node version check from the package.json | |||
There was a problem hiding this comment.
LGTM, @justinfagnani you have an outstanding review, can you PTAL/merge?
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@justinfagnani @FredKSchott Following up on this, I have resolved the changelog conflicts |
|
Thanks @justinfagnani looks like the build failed on |
stramel
force-pushed
the
ms/node-version-check
branch
2 times, most recently
from
a8808ff
to
f03e3a0
Compare
stramel
force-pushed
the
ms/node-version-check
branch
from
f03e3a0
to
959b1bc
Compare
|
Any chance we can get this merged? I just rebased again. I also regenerated the package-lock.json since I had conflicts in there and none seemed to be any breaking changes. |
|
@stramel merged! |
