-
Notifications
You must be signed in to change notification settings - Fork 142
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
Support Node 16 #965
Support Node 16 #965
Changes from 39 commits
298e638
906b23e
b8d680d
fa17a85
86f9353
3eb9ee1
c9e24b6
e81ea18
74081e3
4b463a5
5c23e0e
261d49a
2e88658
6684628
5678a28
4f3dd08
614549f
5182c84
60ae753
3c2e5a4
ea563c5
40be1e2
942dad5
cdbe418
fdc6d21
70e1bdf
7a7567a
58f8ca4
35dad46
b571ee7
bdf0896
f73e739
8078e1b
25dc0d6
5674fe6
0f2cf9d
1298a0f
f123067
d6e14c1
3eb363b
d90306f
85fbe74
5c7552a
0dcfef7
e7f7df6
f230864
0c50c68
ee119ad
0e6e00e
b7186e8
38dbed1
5bc8c36
570c53d
87cf7eb
cba5cb5
85cd2dc
c08035f
6ec6d7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,14 +30,13 @@ jobs: | |||||||||||
pwa-kit: | ||||||||||||
strategy: | ||||||||||||
matrix: | ||||||||||||
node: [14] | ||||||||||||
node: [14, 16] | ||||||||||||
npm: [6, 7, 8] | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. Node 16 requires npm7. Let's skip the Node 16 and npm 6 build from the matrix:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see what you mean - node 16 comes with npm 7 or 8, so there's no point testing node 16 with npm 6. In that case, would we also exclude node 14 with npm 7 and 8? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, let me think a bit more. I'm wrong. We still need to test npm 6 with both Node 14 and Node 16 until Node 14 reaches EOL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, it makes sense that we need to support npm 6 until node 14 reaches EOL. I wonder whether in practice, anyone would use a different version of npm than the one that comes installed with their current node version though 🤔 I will change it back, but something interesting I noticed is that after Brian and I fixed the tests that were consistently failing, several more builds have failed due to flaky tests and (for the most part) they happen on the non-default node/npm combinations (ex. here, here, and here). |
||||||||||||
runs-on: ubuntu-latest | ||||||||||||
env: | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since Node 16 is our active version, we should also change the default npm to npm@8. pwa-kit/.github/workflows/test.yml Lines 36 to 40 in 0748de1
|
||||||||||||
# The "default" npm is the one that ships with a given version of node | ||||||||||||
# node v14 uses npm@6, latest node v16 uses npm@8 | ||||||||||||
# The "default" npm is the one that ships with a given version of node. | ||||||||||||
# For more: https://nodejs.org/en/download/releases/ | ||||||||||||
IS_DEFAULT_NPM: ${{ matrix.npm == 6 }} | ||||||||||||
IS_DEFAULT_NPM: ${{ matrix.node == 14 && matrix.npm == 6 || matrix.node == 16 && matrix.npm == 8 }} | ||||||||||||
steps: | ||||||||||||
- name: Checkout | ||||||||||||
uses: actions/checkout@v3 | ||||||||||||
|
@@ -118,11 +117,13 @@ jobs: | |||||||||||
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} | ||||||||||||
pwa-kit-windows: | ||||||||||||
strategy: | ||||||||||||
# TODO: We don't *need* a matrix with single values, | ||||||||||||
# but is it worth keeping for supporting multiple versions in the future? | ||||||||||||
matrix: | ||||||||||||
node: [14] | ||||||||||||
npm: [6] | ||||||||||||
matrix: | ||||||||||||
node: [14, 16] | ||||||||||||
npm: [6, 7, 8] | ||||||||||||
env: | ||||||||||||
# The "default" npm is the one that ships with a given version of node. | ||||||||||||
# For more: https://nodejs.org/en/download/releases/ | ||||||||||||
IS_DEFAULT_NPM: ${{ matrix.node == 14 && matrix.npm == 6 || matrix.node == 16 && matrix.npm == 8 }} | ||||||||||||
runs-on: windows-latest | ||||||||||||
steps: | ||||||||||||
- name: Checkout | ||||||||||||
|
@@ -134,6 +135,11 @@ jobs: | |||||||||||
node-version: ${{ matrix.node }} | ||||||||||||
cache: npm | ||||||||||||
|
||||||||||||
- name: Update NPM version | ||||||||||||
if: env.IS_DEFAULT_NPM == 'false' | ||||||||||||
run: |- | ||||||||||||
npm install -g npm@${{ matrix.npm }} | ||||||||||||
|
||||||||||||
- name: Setup Windows Machine | ||||||||||||
uses: "./.github/actions/setup_windows" | ||||||||||||
|
||||||||||||
|
@@ -157,7 +163,7 @@ jobs: | |||||||||||
- name: Setup Node | ||||||||||||
uses: actions/setup-node@v3 | ||||||||||||
with: | ||||||||||||
node-version: 14 | ||||||||||||
node-version: 16 | ||||||||||||
|
||||||||||||
- name: Setup Ubuntu Machine | ||||||||||||
uses: "./.github/actions/setup_ubuntu" | ||||||||||||
|
@@ -247,7 +253,7 @@ jobs: | |||||||||||
- name: Setup Node | ||||||||||||
uses: actions/setup-node@v3 | ||||||||||||
with: | ||||||||||||
node-version: 14 | ||||||||||||
node-version: 16 | ||||||||||||
|
||||||||||||
- name: Setup Windows Machine | ||||||||||||
uses: "./.github/actions/setup_windows" | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
"author": "[email protected]", | ||
"license": "See license in LICENSE", | ||
"engines": { | ||
"node": "^14.0.0", | ||
"node": "^14.0.0 || ^16.0.0", | ||
"npm": "^6.14.4 || ^7.0.0 || ^8.0.0" | ||
}, | ||
"files": [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,7 @@ const p = require('path') | |
const sh = require('shelljs') | ||
const fs = require('fs') | ||
const cp = require('child_process') | ||
const semver = require('semver') | ||
|
||
sh.set('-e') | ||
|
||
|
@@ -147,7 +148,12 @@ const runGenerator = () => { | |
// Shelljs can't run interactive programs, so we have to switch to child_process. | ||
// See https://github.com/shelljs/shelljs/wiki/FAQ#running-interactive-programs-with-exec | ||
|
||
cp.execSync(`npx pwa-kit-create-app ${process.argv.slice(2).join(' ')}`, { | ||
const extension = process.platform === 'win32' ? '.cmd' : '' | ||
const npm = `npm${extension}` | ||
const foundNpm = cp.spawnSync(npm, ['-v']).stdout.toString().trim() | ||
const flags = semver.satisfies(foundNpm, '>=7') ? '-y' : '' | ||
Comment on lines
+151
to
+154
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
cp.execSync(`npx ${flags} pwa-kit-create-app@latest ${process.argv.slice(2).join(' ')}`, { | ||
stdio: 'inherit' | ||
}) | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also trigger a Node 16 build on Windows by adding 16 to the windows job matrix, please?
pwa-kit/.github/workflows/test.yml
Lines 119 to 124 in 8b235fa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Waiting to see if the tests pass 🤞🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, @raiyaj since Node 16 is the active Node version, Do you mind also updating the generated builds to use Node 16?
pwa-kit/.github/workflows/test.yml
Line 160 in ea563c5
pwa-kit/.github/workflows/test.yml
Line 250 in ea563c5