fix: async not transpiled on babel for older browser #174
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.
| updateContext: (context) => client.current.updateContext(context), | |
| updateContext: (context) => client.current.updateContext(context), |
There was a problem hiding this comment.
Hi @pr28jat I can't spot any difference in this suggestion? Can you try #174 (comment) ?
There was a problem hiding this comment.
Hi @gastonfournier , sorry for delay. I've updated it as suggested. Thanks!
There was a problem hiding this comment.
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.
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.
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.
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.
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 🙏
There was a problem hiding this comment.
I'm sorry @gastonfournier I just saw it and just merged it today. Thanks again for the PR!
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
| @@ -148,7 +148,7 @@ test('A consumer should be able to get a variant when the client is passed into | |||
| 'consuming value isEnabled true' | |||
| ); | |||
| expect(screen.getByText(/consuming value updateContext/)).toHaveTextContent( | |||
| 'consuming value updateContext [object Promise]' | |||
| 'consuming value updateContext undefined' | |||
There was a problem hiding this comment.
Now the promise is consumed and because setting the context returns no value, this seems correct to me
There was a problem hiding this comment.
It will change the SDK behavior in a subtle way. We have a this binding in client.updateContext. I'm against changing it for the sake of end-of-life browsers.
async syntax is equivalent to
updateContext: (context) => new Promise(
(resolve) => (client.current.updateContext(context))?.then(resolve)
),
| @@ -148,7 +148,7 @@ test('A consumer should be able to get a variant when the client is passed into | |||
| 'consuming value isEnabled true' | |||
| ); | |||
| expect(screen.getByText(/consuming value updateContext/)).toHaveTextContent( | |||
| 'consuming value updateContext [object Promise]' | |||
| 'consuming value updateContext undefined' | |||
There was a problem hiding this comment.
It will change the SDK behavior in a subtle way. We have a this binding in client.updateContext. I'm against changing it for the sake of end-of-life browsers.
async syntax is equivalent to
updateContext: (context) => new Promise(
(resolve) => (client.current.updateContext(context))?.then(resolve)
),
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
About the changes
Closes #173
Important files
Discussion points
Technically if updateContext have nothing to return, then we don't need to use
asyncas well to wrap the function here. CMIIW.