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

Upgrade Undici to v6.7.0 #85

Merged
merged 2 commits into from
Apr 2, 2024
Merged

Conversation

cosieLq
Copy link
Contributor

@cosieLq cosieLq commented Mar 5, 2024

Closes #82

This PR tries to upgrade Undici to v6.7.0. I haven't run integration tests, and I'm curious if everything is still working well.

This is my first commit to this repo, and comments are very welcome! ☺️

Copy link

cla-checker-service bot commented Mar 5, 2024

💚 CLA has been signed

@@ -165,6 +164,7 @@ export default class Connection extends BaseConnection {
if (timeoutId != null) clearTimeout(timeoutId)
switch (err.code) {
case 'UND_ERR_ABORTED':
case DOMException.ABORT_ERR:
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you added DOM support? This library is not intended to be run in a browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Undici's request seems to possibly throw a DOMException also in the node environment, which causes the unit test to fail if the DOMException is not caught here.

I see in Undici's own test that they test it against DOMException as well: https://github.com/nodejs/undici/blob/main/test/node-test/abort-controller.js (Line 126).

Node.js supports DOMException in the globals since v17. That's why I added it here.

@thomasconner
Copy link

Any update on this?

@JoshMock
Copy link
Member

Finally got a chance to pull this down and test it against the client. All unit and integration tests pass smoothly! Just need @cosieLq to sign that CLA and I can merge this for the next minor release.

@JoshMock
Copy link
Member

@cosieLq Additionally, if you merge main into this branch, those Node 16 tests will be removed.

@JoshMock JoshMock self-assigned this Mar 28, 2024
@JoshMock JoshMock mentioned this pull request Mar 28, 2024
@cosieLq
Copy link
Contributor Author

cosieLq commented Apr 2, 2024

@cosieLq Additionally, if you merge main into this branch, those Node 16 tests will be removed.

@JoshMock I've signed the CLA and merged main to this branch. Thank you for your reviewing and testing!

@JoshMock JoshMock merged commit 8856e62 into elastic:main Apr 2, 2024
12 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.

Support Undici v6
3 participants