Skip to content
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.

Sign up for GitHub

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

Jump to bottom

build: update minimum supported Node version from 16.13.0 -> 16.14.0 #49771

Closed
wants to merge 1 commit into from
Closed

build: update minimum supported Node version from 16.13.0 -> 16.14.0 #49771

wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

This commit updates the minimum supported Node version across packages from 16.13.0 -> 16.14.0 to ensure compatibility with dependencies.

PR Type

What kind of change does this PR introduce?

  • Build related changes

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added area: build & ci Related the build and CI infrastructure of the project action: review The PR is still awaiting reviews from at least one requested reviewer target: rc This PR is targeted for the next release-candidate labels Apr 10, 2023
@ngbot ngbot bot modified the milestone: Backlog Apr 10, 2023
@pullapprove pullapprove bot requested a review from alxhub April 10, 2023 20:39
@JeanMeche
Copy link
Member

Aren't you missing the change in .circleci/config.yml ?

@AndrewKushnir
build: update minimum supported Node version from 16.13.0 -> 16.14.0
9ade052 
This commit updates the minimum supported Node version across packages from 16.13.0 -> 16.14.0 to ensure compatibility with dependencies.
josephperrott
josephperrott approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/animations/package.json
@@ -5,7 +5,7 @@
"author": "angular",
"license": "MIT",
"engines": {
"node": "^16.13.0 || >=18.10.0"
"node": "^16.14.0 || >=18.10.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary for us to bump this version since our users don't have to rely on the package we are seeing issue with?

I am happy to have us bump to a later node version if we want, but I don't know that its strictly necessary for our own issue we are seeing.

cc: @alxhub

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen that lru-cache 9.0.0 is pulled by @angular-devkit/build-angular with a project on next and v15. But I couldn't trigger that error. Any idea which functionality triggers it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josephperrott , lru-cache cache is a widely used package and is used by a number of our deps (CLI) and is a matter of time until one of our transitive dependencies consumes it. Hence, to keep the node versions in sync with the CLI, I suggest to bump it.

Hence, it would be best to bump since bumping to 16.14.0 might be too distributive in a minor/patch version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lru-cache ends up being downloaded as a dependency when you install @angular-devkit/build-angular, and if you are running a lower version of node (i.e. 16.13.0) it fails the engines check during install.

nvm install 16.13.0;
npx @angular/cli new test_project

You will see the unsupported engine message on install attempt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense for me to do in v16-next

josephperrott
josephperrott approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: global-approvers

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 11, 2023
@ngbot
Copy link

ngbot bot commented Apr 11, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    pending status "ci/circleci: setup" is pending
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@AndrewKushnir
Copy link
Contributor Author

This PR was merged into the repository by commit b98ecbc.

AndrewKushnir added a commit that referenced this pull request Apr 11, 2023
@AndrewKushnir
build: update minimum supported Node version from 16.13.0 -> 16.14.0 (#…
898eecb 
…49771)

This commit updates the minimum supported Node version across packages from 16.13.0 -> 16.14.0 to ensure compatibility with dependencies.

PR Close #49771
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alan-agius4 alan-agius4 alan-agius4 approved these changes

@devversion devversion devversion approved these changes

@JeanMeche JeanMeche JeanMeche left review comments

@josephperrott josephperrott josephperrott approved these changes

@alxhub alxhub Awaiting requested review from alxhub

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
5 participants
@AndrewKushnir @JeanMeche @devversion @josephperrott @alan-agius4