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

feat: update default nodejs version to 16 #1138

Merged
merged 5 commits into from Jul 13, 2022
Merged

feat: update default nodejs version to 16 #1138

merged 5 commits into from Jul 13, 2022

Conversation

biruwon
Copy link
Contributor

@biruwon biruwon commented Jul 8, 2022

🎉 Thanks for submitting a pull request! 🎉

Summary

As commented by @Skn0tt, we are updating the default nodejs runtime to version 16 but was missing here. The version will be updated on Monday, so we can release it after that. Let me know if I am doing something wrong as it is my first PR here 😄

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻.
    This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
    a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@biruwon biruwon requested a review from a team July 8, 2022 09:32
@biruwon biruwon added the type: chore work needed to keep the product and development running smoothly label Jul 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2022 •
edited

⏱ Benchmark results

Comparing with 20fe86c

largeDepsEsbuild: 5.7s

⬇️ 2.05% decrease vs. 20fe86c

^   5.9s    5.9s    5.7s  
│   ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
│   |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴──>
    T-2     T-1      T
Legend

largeDepsZisi: 43.1s

^  43.7s           43.1s  
│   ┌──┐            ┌──┐  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |            |▒▒|  
│   |  |    n/a     |▒▒|  
└───┴──┴────────────┴──┴──>
    T-2     T-1      T
Legend

danez
danez previously requested changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@danez danez left a comment •
edited

Choose a reason for hiding this comment

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

I guess we need to coordinate this release with bitballon exactly?

tests/unit/runtimes/node/utils/node_version.js Show resolved Hide resolved
@danez danez changed the title Update default nodejs version to 16 feat: Update default nodejs version to 16 Jul 8, 2022
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Jul 8, 2022 •
edited

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@danez danez added type: feature code contributing to the implementation of a feature and/or user facing functionality and removed type: chore work needed to keep the product and development running smoothly labels Jul 8, 2022
@biruwon
Copy link
Contributor Author

biruwon commented Jul 11, 2022 •
edited by danez

I guess we need to coordinate this release with bitballon exactly?

If I am not wrong, this project prepares the functions to be uploaded as a zip. So, it could have been even merged before as we are just changing the default runtime. But let's do it after the default has been rolled out 100% so we know is not causing any problem, and then we merge the rest.

@biruwon biruwon changed the title feat: Update default nodejs version to 16 Update default nodejs version to 16 Jul 11, 2022
@biruwon biruwon requested a review from danez July 11, 2022 10:36
@biruwon
Copy link
Contributor Author

biruwon commented Jul 11, 2022

BTW @danez, I saw the bot reported like a 50% decreased . I guess that good, but not sure if is it related to the nodejs version or just some random numbers that depends on the time it was executed 🤔

@biruwon
Copy link
Contributor Author

biruwon commented Jul 11, 2022

BTW @danez, I saw the bot reported like a 50% decreased . I guess that good, but not sure if is it related to the nodejs version or just some random numbers that depends on the time it was executed 🤔

I just did another commit and results are totally different, so already answered 😄

@biruwon biruwon dismissed danez’s stale review July 13, 2022 10:02

bitballoon already released

@biruwon biruwon changed the title Update default nodejs version to 16 feat: update default nodejs version to 16 Jul 13, 2022
danez
danez reviewed Jul 13, 2022
View reviewed changes
tests/unit/runtimes/node/utils/node_version.js
t.is(getNodeVersion('nodejs12.x'), 12)
t.is(getNodeVersion('nodejs8.x'), 8)
t.is(getNodeVersion('12.x'), 12)
t.is(getNodeVersion('8.x'), 8)
t.is(getNodeVersion('node14'), DEFAULT_NODE_VERSION)
t.is(getNodeVersion('node16'), DEFAULT_NODE_VERSION)
Copy link
Contributor

@danez danez Jul 13, 2022 •
edited

Choose a reason for hiding this comment

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

I know you jsut changed the number, but it was actually wrong before. node1X is not a valid specifier. It needs to be either nodejs16.x or 16.x. It works though because in case the input cannot be parse it defaults to the default nodejs version. Hence the next line in this file with :shrug:

Suggested change
t.is(getNodeVersion('node16'), DEFAULT_NODE_VERSION)
t.is(getNodeVersion('16.x'), DEFAULT_NODE_VERSION)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this unit test check that something like "nodeVersion" is invalid so it uses the default one. Not sure if we should change it

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I see.

@danez
Copy link
Contributor

danez commented Jul 13, 2022

Just too clarify again, this has nothing todo with the runtime, but with the output target of our bundlers. So with the runtime now being default nodejs 16 in bitballon, but we still bundle the functions here for node14 (which is fine and works, but can be better optimized if we also set it to node16). This PR changes that.

@kodiakhq kodiakhq bot merged commit dec245a into main Jul 13, 2022
@kodiakhq kodiakhq bot deleted the antonio/update-default-nodejs-version branch July 13, 2022 15:12
@token-generator-app token-generator-app bot mentioned this pull request Jul 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danez danez danez approved these changes

Assignees
No one assigned
Labels
automerge type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@biruwon @danez