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 return type of filter #4

Merged
merged 3 commits into from
Mar 30, 2022
Merged

Conversation

jpasquers
Copy link
Contributor

Fixes #3

@wooorm
Copy link
Owner

wooorm commented Feb 9, 2022

Can you look at the tests?

@jpasquers
Copy link
Contributor Author

Can you look at the tests?

Yep I'm looking into it. Sorry been in on and off meetings but will have some time in a bit

@wooorm
Copy link
Owner

wooorm commented Feb 9, 2022

all the time in the world, no rush, and let me know if you have Qs!

@jpasquers
Copy link
Contributor Author

So this led me down quite the rabbit hole. As far as I could tell the ts typing was failing even in main. There is some really tricky behavior surrounding the filter option dictating the return type of the inner match method. I reached a solution which is reasonable but probably overly complicated. An additional consequence of the new type declarations is the unintended export of things like FilterOrLookup and FilterOrLookupResponse

@jpasquers
Copy link
Contributor Author

We can go forward with this, but I definitely understand if you would rather just reformat the code or have any other insights. I just wanted to avoid touching the real code if possible.

@jpasquers
Copy link
Contributor Author

any update here?

Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Thanks for your patience!

As far as I could tell the ts typing was failing even in main

I see it, weird how that started happening 🤷‍♂️

An additional consequence of the new type declarations is the unintended export of things like FilterOrLookup and FilterOrLookupResponse

This can be “solved” by moving everything to lib/, and explicitly choosing which types and identifiers to export from index.js.

I just wanted to avoid touching the real code if possible.

👍


I’ll merge this, and will take a look at the code-style and what to export locally!

@wooorm wooorm changed the title update return type declaration Fix return type of filter Mar 30, 2022
@wooorm wooorm merged commit 392cd02 into wooorm:main Mar 30, 2022
@wooorm
Copy link
Owner

wooorm commented Mar 30, 2022

Managed to remove a bunch of types that weren’t needed, and was able to get rid of the prettier-ignore: 6d4a74b?w=1

@wooorm
Copy link
Owner

wooorm commented Mar 30, 2022

Thanks, released in 2.0.2!

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.

Filter return type
2 participants