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 for #357 #358

Closed
wants to merge 1 commit into from
Closed

Fix for #357 #358

wants to merge 1 commit into from

Conversation

rubenfiszel
Copy link

@CGNonofr I tried to guess how to do an escape hatch but the whole bypassConversion check seems flaky to me. I believe it might make more sense to do a blacklist rather than the current whitelist

@CGNonofr
Copy link
Collaborator

Indeed it's not very robust! well spotted, a refactor is probably needed here

@CGNonofr
Copy link
Collaborator

CGNonofr commented May 23, 2022

I did another PR: #359

It includes some typings to be sure we're only returning a promise when it is expected, what you think?

@CGNonofr
Copy link
Collaborator

It was fixed by #359, thank you very much for your investigations!

@rubenfiszel
Copy link
Author

Glad to see a clean fix! You use some advanced typing that I'm actually really curious about, will dig into it. Do you plan to do a release soon with your fix ?

@kaisalmen
Copy link
Collaborator

@CGNonofr
Copy link
Collaborator

Glad to see a clean fix! You use some advanced typing that I'm actually really curious about, will dig into it.

There is probably a simpler way to do it tho! I don't know why Exclude wasn't working

Do you plan to do a release soon with your fix ?

I think it will come soon by @kaisalmen

@rubenfiszel
Copy link
Author

I updated and it did the trick for #356, the dev experience is actually pretty amazing now, thanks a lot!

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.

None yet

3 participants