-
Notifications
You must be signed in to change notification settings - Fork 64
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 for NodeJS v18 #567
Conversation
nvm version | ||
nvm install 18.15.0 | ||
nvm use 18.15.0 | ||
nvm ls |
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.
Is this here as a debug step?
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.
Yes it is. It lists all the installed Node versions and highlights the default. We can get rid of it but I don't see any risk in keeping it.
command: | | ||
nvm version | ||
nvm install 18.15.0 | ||
nvm use 18.15.0 |
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.
Are there any differences in behavior between use and alias default? e.g. if I create a new shell I'd expect that the use
wouldn't be respected anymore, right?
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.
Actually, can we just use the same node/install command from down below? Or does that not work on the windows executor?
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.
I added the node/install
step for the android executor because it does not have nvm
included. https://github.com/aws-amplify/amplify-codegen/blob/main/scripts/split-e2e-tests.ts#L135
I can't remember what happened when trying node/install on all the jobs, but it would be worth trying to possibly simplify the config.
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.
I've tried using the node
orb for windows and it fails. Refer this job for error info.
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.
$ nvm alias default 18.15.0
$ nvm use
is identical to nvm use 18.15.0
since we currently we run the test job in a single shell on windows. The later is more concise.
- run: yarn config set workspaces-experimental true | ||
- run: yarn cache clean --force |
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.
Why the change to workspaces and cache here?
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.
The updated dependencies were not being reflected due to cache for the Android container. This causes the build-app-android-test
job to fail. Refer error logs in this failed run of the test.
Description of changes
Issue #, if available
Description of how you validated changes
Existing tests run successfully post upgrade.
https://app.circleci.com/pipelines/github/aws-amplify/amplify-codegen/2899/workflows/2f10290a-4b85-460e-9606-a8b85a9e3e70
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.