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

Adding support for nodejs20 #683

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

NiketaPopatChaudhari
Copy link

@NiketaPopatChaudhari NiketaPopatChaudhari commented Apr 10, 2024

What did you implement:
Adding support for Nodejs 20

How can we verify it:
Use it to deploy a nodejs20 function app to Azure.

Todos:
Note: Run npm run test:ci to run all validation checks on proposed changes

Ensure there are no lint errors.
Validate via npm run lint
Note: Some reported issues can be automatically fixed by running npm run lint:fix

Validate via npm test

Is this ready for review?: YES
Is it a breaking change?: NO

@gligorkot gligorkot self-assigned this Apr 15, 2024
@gligorkot gligorkot self-requested a review April 15, 2024 22:57
@gligorkot
Copy link
Contributor

@NiketaPopatChaudhari can you please add a validate for version 20 here:

and integration for version 20 in 2 places:

@NiketaPopatChaudhari
Copy link
Author

version 20 added in validate.yml and integration.yml

@NiketaPopatChaudhari
Copy link
Author

Done

@gligorkot
Copy link
Contributor

@NiketaPopatChaudhari looks like validate failed for node 20, can you please have a look, the failures can be found here https://github.com/serverless/serverless-azure-functions/actions/runs/8715495822/job/23909330268?pr=683

@NiketaPopatChaudhari
Copy link
Author

Looking into @gligorkot

@gligorkot
Copy link
Contributor

@NiketaPopatChaudhari you can try run the tests locally

@NiketaPopatChaudhari
Copy link
Author

yes @gligorkot i am testing it on local and working on it to fix UT

@NiketaPopatChaudhari
Copy link
Author

NiketaPopatChaudhari commented Apr 22, 2024

@gligorkot have tested on local with master branch Observation as below

Node Version 18.20.2 without any changes on the master branch

image

Node Version 20.12.1 without any changes on the master branch

image

@gligorkot
Copy link
Contributor

@NiketaPopatChaudhari make sure you have cleared node_modules and re-installed them when running with node 18.

When I run node 18.20.2, all tests pass on the master branch for me:
Screenshot 2024-04-22 at 11 19 33 PM

However when I run using node 20.12.2 some of them do fail - can you please investigate what fails and what we need to do to fix these?
Screenshot 2024-04-22 at 11 22 18 PM

@yashGanatraHK
Copy link

Hey @gligorkot, @NiketaPopatChaudhari

I tried executing Tests (with Node Version: 20.12.2) in master branch (Without any changes) and it still fails in 27 cases.
After having some research I could find out that its the test case code which is creating the issue here:

Most/Almost All of the failed cases are related to Mock FS module. Mock FS is already having Open issue with Node 20.
Ref: tschaub/mock-fs#384

We can try to change MockFs to MemFs but it will take some time to make these changes.

@gligorkot
Copy link
Contributor

Hey @yashGanatraHK, thank you for looking into this issue. I'd first look at if removing mock-fs altogether is a viable approach, and failing that, yes updating to memfs would be my second choice.

@NiketaPopatChaudhari
Copy link
Author

@gligorkot Have replaced all mockFs with memfs and made some additional changes now fail test count as below
image

@gligorkot
Copy link
Contributor

@gligorkot Have replaced all mockFs with memfs and made some additional changes now fail test count as below
image

Thanks for keeping me in the loop @NiketaPopatChaudhari. Could you please have a look at what else needs changing to fix the remaining tests?

@jonatandorozco
Copy link

@gligorkot @NiketaPopatChaudhari please let me know if there is a way I can help with this

@gligorkot
Copy link
Contributor

@gligorkot @NiketaPopatChaudhari please let me know if there is a way I can help with this

@jonatandorozco would you like to fork Niketa's branch and finish off the implementation then raise a PR for this?

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.

4 participants