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: typescript issue related to aedes.handle and client #802

Conversation

haci-xplora
Copy link

ref: issue 801

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

No need to set it to null simply add the ? parameter, when omitted it will be undefined

@haci-xplora
Copy link
Author

to specify optional parameter in JavaScript, I have to assign it to null and then go and fix that in TypeScript type definition accordingly.

what do you think?

@haci-xplora
Copy link
Author

also we should not use expressions like {} as IncomingMessage in some tests. we can use null now.

shall I fix that as well?

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

I dunno what you mean, does using the changes I suggest here creates errors somewhere?

types/instance.d.ts Outdated Show resolved Hide resolved
types/client.d.ts Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
aedes.js Outdated Show resolved Hide resolved
@haci-xplora
Copy link
Author

if you do not want to change javascript files, but only typescript files, that would work. but then the discrepancy between the code and the documentation stays.

@robertsLando robertsLando changed the title 801 fixing typescript issue related to aedes.handle and client fix: typescript issue related to aedes.handle and client Jan 2, 2023
@haci-xplora
Copy link
Author

changes on js files undone.

@haci-xplora
Copy link
Author

almost undefined vs null

it's been an interesting discussion

@coveralls
Copy link

coveralls commented Jan 3, 2023

Pull Request Test Coverage Report for Build 3823427449

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.739%

Totals Coverage Status
Change from base Build 3702154582: 0.0%
Covered Lines: 816
Relevant Lines: 816

💛 - Coveralls

@robertsLando
Copy link
Member

robertsLando commented Jan 3, 2023

@haci-xplora I think it may make sense to also add a types test here that also calls handle without the req to prevent regressions

After that this is good to merge 👍🏼

@robertsLando
Copy link
Member

robertsLando commented Mar 1, 2023

Superseded by #812

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

Successfully merging this pull request may close these issues.

None yet

3 participants