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.

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

feat: bump node version to 18.17.1 #319

Merged

Conversation

pac-guerreiro
Copy link
Contributor

Details

Onyx currently enforces developers to use NodeJS 16.15.1. As of September, NodeJS 16.x will reach its End-of-life (EOL), meaning there will be no more updates, patches, or security fixes. Additionally, reactwg/react-native-releases#64.

Using an outdated version of NodeJS not only exposes the application to potential security vulnerabilities, compatibility issues, and prevents developers from leveraging the latest features, optimizations, and improvements available in newer versions of NodeJS.

Related Issues

Automated Tests

Not necessary since this PR only changes project configuration files.

Linked PRs

None.

@pac-guerreiro pac-guerreiro requested a review from a team as a code owner August 29, 2023 17:34
@melvin-bot melvin-bot bot requested review from bondydaa and removed request for a team August 29, 2023 17:35
bondydaa
bondydaa previously approved these changes Aug 29, 2023
Copy link
Contributor

@bondydaa bondydaa left a comment

Choose a reason for hiding this comment

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

👍 seems right to me but going to assign some others who are more familiar as well.

package.json Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor

I think this should be subject to the merge freeze, putting it on HOLD. @pac-guerreiro please take this off HOLD when the merge freeze is lifted

@roryabraham roryabraham changed the title feat: bump node version to 18.17.1 [HOLD] feat: bump node version to 18.17.1 Aug 30, 2023
bondydaa
bondydaa previously approved these changes Sep 12, 2023
Copy link
Contributor

@bondydaa bondydaa left a comment

Choose a reason for hiding this comment

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

👍 again, i think feature freeze is over so taking off hold but @roryabraham you want to double check this and merge if you feel 👍 about it?

@bondydaa bondydaa changed the title [HOLD] feat: bump node version to 18.17.1 feat: bump node version to 18.17.1 Sep 12, 2023
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

LGTM as long as we follow-up to remove the range and pin to 18

@roryabraham
Copy link
Contributor

However, I can't merge this PR due to it containing unsigned commits. @pac-guerreiro, please set up GPG signing for your commits and register your GPG key with GitHub, then rebase this PR, reword each commit, and force-push.

image

roryabraham
roryabraham previously approved these changes Sep 15, 2023
@roryabraham
Copy link
Contributor

Tests are failing

@parasharrajat
Copy link
Member

So react-native-performance was upgraded to v5.x recently #340. Can that be related to test failure?

@fabioh8010
Copy link
Contributor

Just a reminder that @pac-guerreiro is OOO at the moment and will return on 20/09.

@pac-guerreiro
Copy link
Contributor Author

@roryabraham

This test is failing but it's unrelated to my changes:

Screenshot 2023-09-21 at 14 23 24

@roryabraham
Copy link
Contributor

@pac-guerreiro tests appear to be passing on main, please try merging main into this branch?

@pac-guerreiro
Copy link
Contributor Author

@roryabraham

I just rebased the branch with main but the issue persists, although locally the tests are passing. Maybe this is a cache issue? Could you re-run the workflow and maybe delete the cache?

@roryabraham
Copy link
Contributor

Deleted caches and re-running

@pac-guerreiro
Copy link
Contributor Author

@roryabraham

I think I might have found why tests are failing:

Screenshot 2023-10-02 at 15 53 00
Screenshot 2023-10-02 at 15 53 38

It seems that we're using an old version of actions/setup-node@v1 and I'm trying to use a feature to read the node version from a config file which is only available in actions/setup-node@v3

@AndrewGable
Copy link
Contributor

Can you update it in this PR and see if the tests pass?

@roryabraham
Copy link
Contributor

For setup-node feel free to pin it directly to the @V3 tag instead of a commit hash

@pac-guerreiro
Copy link
Contributor Author

pac-guerreiro commented Oct 5, 2023

@roryabraham I'm having issues with GPG. Somehow I'm unable to commit anything, even after generating a new key 😢

Solved!

@pac-guerreiro
Copy link
Contributor Author

@AndrewGable @roryabraham All checks have passed! 😄

@bondydaa
Copy link
Contributor

bondydaa commented Oct 9, 2023

going to merge this so it doesn't get tripped up by some other conflict, looks like rory had previously approved it but we didn't merge b/c of failing tests that are now fixed.

@bondydaa bondydaa merged commit b7849bd into Expensify:main Oct 9, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants