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

feat(feature-flags): Add bootstrap option for feature flags #24

Merged
merged 12 commits into from
Sep 9, 2022

Conversation

neilkakkar
Copy link
Contributor

Just like PostHog/posthog-js#444 but for react-native.

@benjackwhite I could use some help figuring out this works for react-native.

Current issues I'm facing:

  1. The CI doesn't seem to be testing posthog-react-native ?
  2. How do I get the new test created specifically for react-native to run? Unfamiliar with expo, and it's complaining about those imports.
  3. Can you help testing a 'live' version of this and confirming it works?

@neilkakkar neilkakkar marked this pull request as draft September 8, 2022 13:35
@benjackwhite
Copy link
Collaborator

1. The CI doesn't seem to be testing `posthog-react-native` ?

It does - It's right after the standard tests https://github.com/PostHog/posthog-js-lite/blob/master/.github/workflows/ci.yml#L22

2. How do I get the new test created specifically for react-native to run? Unfamiliar with expo, and it's complaining about those imports.

Expo is a hard beast to test but for this we can stick to the unit tests. They have to be run separate to the other tests due to some specific tsconfig rules for React Native...

3. Can you help testing a 'live' version of this and confirming it works?

Sure - I'll check it out!

@benjackwhite
Copy link
Collaborator

@neilkakkar trying to fix the tests but not sure what the logic should be.
Currently its doing this:

  1. On load bootstrap the things
  2. If feature flags are given they are set 👍
  3. Immediately after we reload the feature flags

My assumption is we don't want to immediately reload the flags from the API if they are given. Is that correct?

@neilkakkar
Copy link
Contributor Author

My assumption is we don't want to immediately reload the flags from the API if they are given. Is that correct?

No, this is okay!

The idea is that some flags are available on immediate load, while we wait for /decide or new properties to reflect in the feature flags.
Some flags would be impossible to bootstrap, hence the immediate reload ensures we get those as soon as possible as well.

@benjackwhite benjackwhite marked this pull request as ready for review September 9, 2022 10:35
@benjackwhite
Copy link
Collaborator

@neilkakkar this looks good now and everything passes. Turns out memorystorage for RN was always broken anyway so this is a good catch...

Might be worth a quick glance for yourself over what I changed but if that's all good then I'd say we are good to go

@neilkakkar
Copy link
Contributor Author

Much appreciated, thanks! I just added a test for the async storage as well.

@neilkakkar neilkakkar merged commit d99dc08 into master Sep 9, 2022
@neilkakkar neilkakkar deleted the bootstrapping branch September 9, 2022 12:27
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.

2 participants