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

Node.js 12 support? #1645

Closed
dominykas opened this issue Sep 13, 2022 · 5 comments · Fixed by #1646
Closed

Node.js 12 support? #1645

dominykas opened this issue Sep 13, 2022 · 5 comments · Fixed by #1646
Labels
bug Something isn't working

Comments

@dominykas
Copy link
Member

dominykas commented Sep 13, 2022

It seems as part of #1604, Node.js 12 was removed from the support matrix - not sure if that was intentional?

exclude: |
- runs-on: windows-latest
node-version: 16
- node-version: 12

If Node.js 12 is no longer supported - then probably the engines field needs to be updated (incl. a major version release) - but quite possibly that was a typo and the matrix needs to be adjusted?

@dominykas dominykas added the bug Something isn't working label Sep 13, 2022
@mcollina
Copy link
Member

this should still support Node v12. I didn't spot this in that PR, I thought we were just skipping it on Windows.

Would you mind sending a PR?

@dominykas
Copy link
Member Author

Seems there's a test failing (test/request-timeout.js) - I can take a look later.

@dominykas
Copy link
Member Author

OK, I think I don't have enough context, TBH. But that test is failing due to https://github.com/nodejs/undici/pull/1604/files#diff-ca407d959d33ce92a7f7efee354a5ea5e6e61fcd797d32c04205e429a074ed54R892 - seems that timeout happens in node 14+ due to socket.writableNeedDrain - which actually makes 14.17.0+ a requirement for the test, but I'd probably need significantly more time to figure out whether the implementation is the right one 😅

Happy to follow any pointers, though...

@mcollina
Copy link
Member

I think that's the best outcome. Just make that test run on Node v14.17+.

@dominykas
Copy link
Member Author

Updated the PR, also had to disable jest for Node 12 - seems that it's accumulating the disabled tests - not sure if that's becoming problematic or not just yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants