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

build : add Node.js@19.7 #5028

Merged
merged 1 commit into from
Feb 23, 2023
Merged

build : add Node.js@19.7 #5028

merged 1 commit into from
Feb 23, 2023

Conversation

abenhamdine
Copy link
Contributor

@abenhamdine abenhamdine commented Oct 19, 2022

for nodejs version available in Github Actions, see https://github.com/actions/node-versions/releases
nodejs versions available in appveyor are slightly different, see https://www.appveyor.com/docs/linux-images-software/#node-js

@abenhamdine
Copy link
Contributor Author

abenhamdine commented Oct 20, 2022

One test is failing on appveyor :

image

@krzysdz
Copy link
Contributor

krzysdz commented Oct 20, 2022

Node.js 19.0 has updated V8 version, which had changed JSON.parse error messages.

Relevant Node.js commit: nodejs/node@71c193e

@RobinTail
Copy link
Contributor

Are any further works expected in this direction? @abenhamdine

@RobinTail
Copy link
Contributor

RobinTail commented Nov 1, 2022

Just in case, this is how I improved the test in my project to support Node 19, @abenhamdine :

expect.stringMatching(
  // the 2nd option is for Node 19
  /(Unexpected end of JSON input|Unterminated string in JSON at position)/
)

@abenhamdine
Copy link
Contributor Author

Just in case, this is how I improved the test in my project to support Node 19, @abenhamdine :

expect.stringMatching(
  // the 2nd option is for Node 19
  /(Unexpected end of JSON input|Unterminated string in JSON at position)/
)

Thx I will give a look when I have some time

@dougwilson
Copy link
Contributor

So there was a change in v8 for how it returns json errors. I have a test change in body-parser to account for this, which is more than a test change. We'll need to wait for that to be finished and released in body-parser before we can then add node 19 to the build of express.

Perhaps then this pr can just be limited to node 18 to get it landed, otherwise just waiting for that dependency update is all the is needed.

@abenhamdine abenhamdine changed the title build : Node.js@18.11 & Node.js@19.0 build : Node.js@18.12 & Node.js@19.0 Nov 1, 2022
@abenhamdine
Copy link
Contributor Author

Perhaps then this pr can just be limited to node 18 to get it landed, otherwise just waiting for that dependency update is all the is needed.

Ok I will open a separate PR for node 18.12 update and keep this one for node 19.

@abenhamdine abenhamdine changed the title build : Node.js@18.12 & Node.js@19.0 build : Node.js@16.18, Node.js@18.12 & Node.js@19.0 Nov 1, 2022
@import-brain import-brain added the pr label Nov 2, 2022
@abenhamdine
Copy link
Contributor Author

FI the node 19 error is fixed upstream in https://github.com/expressjs/body-parser/releases/tag/1.20.2

@abenhamdine
Copy link
Contributor Author

abenhamdine commented Feb 22, 2023

I have rebased the branch on top of master, and have updated nodejs versions with last 16 and 18 versions available in GH and appveyor.
However, nodejs 18 and 19 tests fail because of a change in npm "npm ERR! The shrinkwrap option is deprecated"

I will give a look.

@abenhamdine abenhamdine changed the title build : Node.js@16.18, Node.js@18.12 & Node.js@19.0 build : update with last versions Node.js@16.19 and Node.js@18.14, add Node.js@19.7 Feb 22, 2023
@abenhamdine
Copy link
Contributor Author

nodejs 18 tests are passing again, but nodejs 19 tests are still blocked by the same error, even with body-parser 1.20.2 :

image

@dougwilson dougwilson changed the title build : update with last versions Node.js@16.19 and Node.js@18.14, add Node.js@19.7 build : add Node.js@19.7 Feb 23, 2023
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants