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

fix missing client arg #555

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

Ethan-Arrowood
Copy link
Collaborator

closes #554

doc update included in #550

@ronag
Copy link
Member

ronag commented Feb 17, 2021

Would be nice with a test. Is this the only occurance?

@Ethan-Arrowood
Copy link
Collaborator Author

Ethan-Arrowood commented Feb 17, 2021

Yeah only occurrence (did a search for emit('disconnect across the whole repo), most of the tests use neither arguments in the event listener, but there is one test that checks them: https://github.com/nodejs/undici/blob/master/test/pool.js#L39

@ronag
Copy link
Member

ronag commented Feb 17, 2021

In the upgrade test can you add a check for disconnect + correct args. I think you can just register a handler in one of the existing tests.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

@Ethan-Arrowood
Copy link
Collaborator Author

Also found another test for this but its not upgrade related so this new case is a good addition

https://github.com/nodejs/undici/blob/master/test/client.js#L980

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina requested a review from ronag February 17, 2021 17:23
@mcollina
Copy link
Member

I think this is semver-major cc @ronag.

@ronag

This comment has been minimized.

@ronag
Copy link
Member

ronag commented Feb 18, 2021

It is semver major. A fix for #544 which we haven't released,

@ronag ronag added the semver-major Features or fixes that will be included in the next semver major release label Feb 18, 2021
@ronag ronag merged commit 6612d33 into nodejs:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disconnect event missing args
3 participants