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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

fix: async not transpiled on babel for older browser #174

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0529b85
fix: async not transpiled on babel for older browser
chenxeed Aug 6, 2024
bf37fd4
refactor: direct function updateContext
chenxeed Sep 2, 2024
1206a4f
Fix test
gastonfournier Sep 3, 2024
ef26686
Merge pull request #1 from Unleash/bugfix/support-older-browser
chenxeed Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/FlagProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
gastonfournier marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

Suggested change
updateContext: (context) => client.current.updateContext(context),
updateContext: (context) => client.current.updateContext(context),

Copy link

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) ?

Copy link
Author

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!

Copy link

@gastonfournier gastonfournier Sep 2, 2024 •
edited
Loading

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

Copy link
Author

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.

Copy link

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

Copy link

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!

Copy link

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 🙏

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry @gastonfournier I just saw it and just merged it today. Thanks again for the PR!

isEnabled: (toggleName) => client.current.isEnabled(toggleName),
getVariant: (toggleName) => client.current.getVariant(toggleName),
client: client.current,
Expand Down
Loading