Support Node 16 #965

raiyaj · 2023-02-07T17:25:06Z

Add support for node 16 by deleting PerformanceTimer class, and make node 16 the default version for new projects.

Description

Over on MRT, we're working on enabling platform-level support for node 16 apps. It currently doesn't work if the app depends on pwa-kit-runtime because of an issue with PerformanceTimer, our custom wrapper around the builtin node perf_hooks API. It turns out that we don't use PerformanceTimer for anything useful (it gathers metrics that we emit to CloudWatch, but Lambda automatically emits the same metrics to CloudWatch). Since the class is marked as @private, this deletes it (which is not a breaking change) and adds explicit support for node 16.

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

Remove PerformanceTimer and all its usages
Explicitly support node 16 in all package.jsons, and make 16.x the default node version for new projects
Update CircleCI workflow to include node 16 and npm 7 and 8
Update create-mobify-app-dev.js to account for breaking changes in npm 7
Fix (or make more resilient) a handful of retail-react-app tests, which were mostly failing due to race conditions that were uncovered with the switch to node 16, because the test execution environment became faster
Re-introduce changes from Add explicit ws dependency #865
- It was released in an alpha release ([email protected]), but somehow didn't make it into the next normal release, so running npm run tail-logs in a newly-generated project returns an error.

How to Test-Drive This PR

Run npm ci at the root of this repo
Repeat the following for node 14.17.0 (npm 6), node 16.7.0 (npm 7), node 16.11.0 (npm 8), and node 16.19.1 (LTS, npm 8):
- Run nvm use <node-version>, then generate a new project:
```
GENERATOR_PRESET=test-project node packages/pwa-kit-create-app/scripts/create-mobify-app-dev.js --outputDir 
~/generated-test-project
```
- cd to the project, then run npm start. The app should load in the browser without errors.
- Make sure your Runtime Admin API key is set in ~/.mobify, then deploy your bundle to MRT: npm run push -- -s <project-slug> -t <target-slug>
  - In Runtime Admin, make sure the deploy succeeds, that your site works, and that your bundle is running node 16.x
  - For reference, I deployed my generated project bundle to https://zombie-apocalypse.mobify-storefront-staging.com
- Run npm run tail-logs -- -p scaffold-pwa -e production and ensure logs appear after a few seconds.

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

bfeister

@raiyaj we have some other incoming changes like React 18. That one in particular is breaking / not backwards compatible. My main interest is to verify that this is backwards compatible with node 14 so we don't have to do a major version bump?

packages/pwa-kit-runtime/src/utils/ssr-server.test.js

This reverts commit e81ea18.

adamraya · 2023-02-15T23:35:58Z

.github/workflows/test.yml

@@ -30,7 +30,7 @@ jobs:
  pwa-kit:
    strategy:
      matrix:
-        node: [14]
+        node: [14, 16]


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

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]

Done! Waiting to see if the tests pass 🤞🏼

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

node-version: 14

pwa-kit/.github/workflows/test.yml

Line 250 in ea563c5

node-version: 14

adamraya · 2023-02-22T20:03:00Z

packages/pwa-kit-dev/src/configs/webpack/config.js

+    const candidates = [
+        resolve(projectDir, 'node_modules', pkg),
+        resolve(__dirname, '..', '..', 'node_modules', pkg),
+        resolve(__dirname, '..', '..', '..', 'node_modules', pkg)


Good finding @raiyaj!
It looks like we don't need the last one since we saw that generates the wrong path with the duplicated node_modules folder.

Thanks Adam! I was thinking of removing it initially, but I added some logging here (in the Setup Ubuntu Machine step), and saw that when a project is generated in CI, __dirname is /home/runner/work/pwa-kit/pwa-kit/packages/pwa-kit-dev/dist/configs/webpack, whereas locally it's /Users/rjessa/generated-test-project/node_modules/pwa-kit-dev/configs/webpack, so we need to try both options.

I copied the candidates idea from here, where we do something similar.

The method's name findInProjectThenSDK implies two places but we now look at three places.
It can be confusing for anyone else looking at the logic or facing a similar problem in the future.

@raiyaj Do you mind leaving a comment in the code explaining why we need to do this to support NPM 7 and Node 16?

adamraya

One last #965 (comment) to document the change in the Webpack config to provide a bit of context for engineers looking at that in the future.

Thanks @raiyaj, Looks good to me!
Congrats on solving all the issues 🎉

adamraya · 2023-02-23T23:42:29Z

@raiyaj I saw a problem with the change we did to theIS_DEFAULT_NPM env variable.
Now we'll have TWO builds that will run steps that only need to happen once like: Push Bundle to MRT or Publish to NPM.

If we follow the current approach, we need a new env variable to designate the unique/main build and update the conditional to include that env variable and guarantee we run the steps only once.

`pwa-kit` job

pwa-kit/.github/workflows/test.yml

Line 69 in 38dbed1

- name: Create MRT credentials file
pwa-kit/.github/workflows/test.yml

Line 76 in 38dbed1

- name: Push Bundle to MRT (Development)
pwa-kit/.github/workflows/test.yml

Line 83 in 38dbed1

- name: Push Bundle to MRT (Production)
pwa-kit/.github/workflows/test.yml

Line 90 in 38dbed1

- name: Push Bundle to MRT (Commerce SDK React)
pwa-kit/.github/workflows/test.yml

Line 101 in 38dbed1

- name: Publish to NPM

~~#### generated job~~

pwa-kit/.github/workflows/test.yml

Line 216 in 38dbed1

- name: Create MRT credentials file
pwa-kit/.github/workflows/test.yml

Line 223 in 38dbed1

- name: Push Bundle to MRT

Edit: As @raiyaj saw, we don't need to change the generated builds for now.

* develop: Support Node 16 (#965) [W-12450361] Introduce short-circuit method for bypassing auth in Commerce Provider by passing in a fetchedToken (#1010) remove jest-silent-reporter (#1009) Clean up Page Designer Code (#1004) [Shopper Experience] `ImageWithText` component (#991) Feature: Page Designer Carousel Component (#977) [Feature] Page Designer Layout Components WIP (#993) remove updatePw from not implemented list (#996) Back-port Shopper Experience Base Components into Retail Template (#992) # Conflicts: # packages/commerce-sdk-react/package-lock.json # packages/commerce-sdk-react/package.json # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-dev/src/configs/webpack/config.js # packages/template-retail-react-app/package-lock.json

raiyaj added 5 commits February 6, 2023 16:51

add explicit dependency on ws

298e638

add custom docs url for 403 errors

906b23e

remove performance timer

b8d680d

add explicit support for node 16

fa17a85

add explicit pwa-kit-dev support for node 16

86f9353

raiyaj changed the title ~~Support Node 16 in pwa-kit-runtime~~ Support Node 16 Feb 7, 2023

raiyaj added 2 commits February 7, 2023 15:21

add explicit support for node 16 across the board

3eb9ee1

delete performance timer tests

c9e24b6

bfeister reviewed Feb 9, 2023

View reviewed changes

packages/pwa-kit-runtime/src/utils/ssr-server.test.js Show resolved Hide resolved

raiyaj added 4 commits February 9, 2023 14:10

add explicit support for node 18

e81ea18

add node 16 and 18 to test matrix

74081e3

Revert "add explicit support for node 18"

4b463a5

This reverts commit e81ea18.

remove 18 from test matrix

5c23e0e

bfeister mentioned this pull request Feb 10, 2023

[BUG] Node v16.x #795

Closed

bfeister linked an issue Feb 10, 2023 that may be closed by this pull request

[BUG] Node v16.x #795

Closed

raiyaj added 3 commits February 13, 2023 14:42

fix use-product-view-modal test

261d49a

fix use-wishlist test

2e88658

lint

6684628

raiyaj force-pushed the pwa-kit-runtime-node16 branch from 35ca1cd to 6684628 Compare February 14, 2023 23:58

raiyaj added 3 commits February 15, 2023 10:04

fix product-view test

5678a28

lint

4f3dd08

try to fix with-registration test

614549f

raiyaj force-pushed the pwa-kit-runtime-node16 branch from 1474291 to 614549f Compare February 15, 2023 22:54

raiyaj added 2 commits February 15, 2023 15:13

update changelog

5182c84

Merge branch 'develop' into pwa-kit-runtime-node16

60ae753

raiyaj marked this pull request as ready for review February 15, 2023 23:20

raiyaj requested a review from a team as a code owner February 15, 2023 23:20

adamraya reviewed Feb 15, 2023

View reviewed changes

update pwa-kit-dev changelog

3c2e5a4

raiyaj added 7 commits February 21, 2023 17:16

fix sdk dir path

3eb363b

add logs

d90306f

make sure sdkDir resolves to pwa-kit-dev

85fbe74

refactor

5c7552a

return a default path rather than throwing an error

0dcfef7

Merge branch 'develop' into pwa-kit-runtime-node16

e7f7df6

update READMEs and babel configs

f230864

adamraya reviewed Feb 22, 2023

View reviewed changes

raiyaj added 2 commits February 22, 2023 12:37

try to make tests more resilient

0c50c68

lint

ee119ad

adamraya approved these changes Feb 22, 2023

View reviewed changes

raiyaj added 3 commits February 22, 2023 16:21

add comment about looking for pwa-kit-dev node_modules in two places

0e6e00e

try to make tests more resilient

b7186e8

undo unnecessary change

38dbed1

raiyaj added 7 commits February 24, 2023 14:38

ensure pwa-kit publish steps are only run once

5bc8c36

Merge branch 'develop' into pwa-kit-runtime-node16

570c53d

remove node 16 / npm 6 builds

87cf7eb

try to make reset-password test more resilient

cba5cb5

Merge branch 'develop' into pwa-kit-runtime-node16

85cd2dc

try to make registration test more resilient

c08035f

Merge branch 'develop' into pwa-kit-runtime-node16

6ec6d7d

raiyaj merged commit b743269 into develop Feb 27, 2023

raiyaj deleted the pwa-kit-runtime-node16 branch February 27, 2023 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Node 16 #965

Support Node 16 #965

raiyaj commented Feb 7, 2023 •

edited

Loading

bfeister left a comment

adamraya Feb 15, 2023

raiyaj Feb 15, 2023

adamraya Feb 16, 2023

adamraya Feb 22, 2023 •

edited

Loading

raiyaj Feb 22, 2023 •

edited

Loading

adamraya Feb 22, 2023

adamraya left a comment

adamraya commented Feb 23, 2023 •

edited

Loading

	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]

Support Node 16 #965

Support Node 16 #965

Conversation

raiyaj commented Feb 7, 2023 • edited Loading

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

bfeister left a comment

Choose a reason for hiding this comment

adamraya Feb 15, 2023

Choose a reason for hiding this comment

raiyaj Feb 15, 2023

Choose a reason for hiding this comment

adamraya Feb 16, 2023

Choose a reason for hiding this comment

adamraya Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

raiyaj Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

adamraya Feb 22, 2023

Choose a reason for hiding this comment

adamraya left a comment

Choose a reason for hiding this comment

adamraya commented Feb 23, 2023 • edited Loading

pwa-kit job

raiyaj commented Feb 7, 2023 •

edited

Loading

adamraya Feb 22, 2023 •

edited

Loading

raiyaj Feb 22, 2023 •

edited

Loading

adamraya commented Feb 23, 2023 •

edited

Loading

`pwa-kit` job