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

Jump to bottom

[FSSDK-9984] fix: initialization and setUser errors #255

Merged
merged 37 commits into from
Apr 9, 2024
Merged

[FSSDK-9984] fix: initialization and setUser errors #255

merged 37 commits into from
Apr 9, 2024

Conversation

mikechu-optimizely
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely commented Mar 22, 2024 •
edited
Loading

Summary

Test plan

  • Existing unit and integration tests are expected to pass

Issues

  • FSSDK-9984 & FSSDK-9985

@mikechu-optimizely mikechu-optimizely added the bug Something isn't working label Mar 22, 2024
@mikechu-optimizely mikechu-optimizely marked this pull request as ready for review March 25, 2024 13:55
mikechu-optimizely
mikechu-optimizely commented Mar 25, 2024
View reviewed changes
src/hooks.ts Outdated Show resolved Hide resolved
raju-opti
raju-opti requested changes Mar 25, 2024
View reviewed changes
src/client.ts Outdated Show resolved Hide resolved
src/hooks.ts Outdated Show resolved Hide resolved
src/hooks.ts Outdated Show resolved Hide resolved
jaeopt
jaeopt requested changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Clarification regarding null dataReadyPromise.

src/hooks.ts Outdated Show resolved Hide resolved
@mikechu-optimizely
Copy link
Contributor Author

Clarification regarding null dataReadyPromise.

@jaeopt I added an inline comment to clarify, hopefully. DM me on Teams to help me understand the covering case you're seeing.

@mikechu-optimizely
Copy link
Contributor Author

@jaeopt and @raju-opti, Can we do a trio programming session?

@mikechu-optimizely
Copy link
Contributor Author

@jaeopt & @raju-opti When's a good time to have a sync meeting on this?

@mikechu-optimizely
Copy link
Contributor Author

We discussed a better, more proper solution. I'm closing temporarily while I complete the WIP.

@mikechu-optimizely mikechu-optimizely requested review from raju-opti and jaeopt and removed request for jaeopt and raju-opti April 3, 2024 19:12
jaeopt
jaeopt requested changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks good. A few changes suggested. Wondering if we can have good tests covering all different scenarios for this complicated timing dependency.

src/client.ts Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
src/Provider.tsx Outdated Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
@mikechu-optimizely mikechu-optimizely mentioned this pull request Apr 5, 2024
Closed
1 task
@mikechu-optimizely mikechu-optimizely removed the request for review from zashraf1985 April 5, 2024 12:35
@mikechu-optimizely mikechu-optimizely requested review from jaeopt and removed request for raju-opti April 5, 2024 19:42
@mikechu-optimizely mikechu-optimizely mentioned this pull request Apr 5, 2024
Closed
1 task
jaeopt
jaeopt approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

jaeopt
jaeopt requested changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Need clarification about public projectConfig

src/client.ts Outdated Show resolved Hide resolved
jaeopt
jaeopt approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@mikechu-optimizely mikechu-optimizely merged commit fae169f into master Apr 9, 2024
10 checks passed
@mikechu-optimizely mikechu-optimizely deleted the mike/FSSDK-9984/fix-initialization-error branch April 9, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raju-opti raju-opti raju-opti requested changes

@jaeopt jaeopt jaeopt approved these changes

Assignees

@mikechu-optimizely mikechu-optimizely

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mikechu-optimizely @jaeopt @raju-opti