-
Notifications
You must be signed in to change notification settings - Fork 32
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: async not transpiled on babel for older browser #174
base: main
Are you sure you want to change the base?
Conversation
- old browser (chrome v50) gets error from the async await - despite attempt to transpile with babel-plugin-transform-async-to-generator, it still not transpiled on my compiler - solution is to update the codebase, since there is no value to return, might as well directly return the method result if it's a Promise
src/FlagProvider.tsx
Outdated
@@ -107,7 +107,7 @@ const FlagProvider: FC<PropsWithChildren<IFlagProvider>> = ({ | |||
() => ({ | |||
on: ((event, callback, ctx) => client.current.on(event, callback, ctx)) as IFlagContextValue['on'], | |||
off: ((event, callback) => client.current.off(event, callback)) as IFlagContextValue['off'], | |||
updateContext: async (context) => await client.current.updateContext(context), | |||
updateContext: (context) => client.current.updateContext(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateContext: (context) => client.current.updateContext(context), | |
updateContext: (context) => client.current.updateContext(context), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pr28jat I can't spot any difference in this suggestion? Can you try #174 (comment) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gastonfournier , sorry for delay. I've updated it as suggested. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The tests are failing though, I've tried locally and they also fail. Currently, this is not a priority to us because Chrome v50 is really old and doesn't seem to have a huge market share. If you manage to fix the tests in a simple way, we will consider merging this, but we don't want to fix support for an old versions if this risks correctness of our SDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the test-case expects the value to be a promise, which if it's not necessary then I can update the unit test as well.
If you manage to fix the tests in a simple way, we will consider merging this, but we don't want to fix support for an old versions if this risks correctness of our SDK
That's fine for me @gastonfournier , for now I'm using patch-package
to modify the code on node_modules
upon install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked at the test, modified it locally, and all tests go green. I'm curious why the previous behavior was ok... or if it was intentional 2 years ago: iamchanii@5ca3a02#diff-465cbe85eff4124dbbe0c763221ff0ae31086b17e85a53b1dd244397a45da94dR183
In any case both [object Promise]
and undefined
feel wrong, but I think it doesn't hurt to change that (because everything works and that seems to be an edge case).
So something like this: chenxeed#1 would do it. And then I can probably merge this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @gastonfournier , sorry for coming back late.
I've approved your PR. Appreciate for the help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I still don't have permissions to merge it because it's in your repository, would you mind merging it into this PR, I hope that solves the problem 🙏
About the changes
Closes #173
Important files
Discussion points
Technically if updateContext have nothing to return, then we don't need to use
async
as well to wrap the function here. CMIIW.