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

[FSSDK-9654] fix: Default to VUID without user(id) as Provider prop #229

Merged
merged 29 commits into from
Jan 16, 2024

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

  • When OptimizelyProvider is used without the user? or userId? props, we should retrieve and use the VUID for optimizely.setUser()
  • Refactored logic from constructor() of provider into lifecycle method componentDidMount()
  • Test updates to support getVuid()
  • Added types to remove warns.

Test plan

  • Existing and updated unit tests are expected to pass
  • Integration and e2e test should continue to pass
  • I'd like to review with @jaeopt for AAT function reqs.

Issues

  • FSSDK-9654

@mikechu-optimizely
Copy link
Contributor Author

I'll re-open later when I've more progress.

@mikechu-optimizely
Copy link
Contributor Author

I think I'm actually ready for a review of this @raju-opti and @jaeopt.

Sorry, this took so long to untangle this new anon paradigm using vuid.

@mikechu-optimizely
Copy link
Contributor Author

Developer review completed.

@mikechu-optimizely
Copy link
Contributor Author

I have an e2e test config set up and able to demo effect.

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.

The fix for using vuid for null userId looks good to me.
A couple of clarifications regarding backward compatibility.

src/client.ts Outdated Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
Copy link
Contributor Author

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

Thanks @jaeopt . I've replied to your comments.

@raju-opti I'd love your double sign off too on this PR.

src/client.ts Outdated Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
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 43613fc into master Jan 16, 2024
7 checks passed
@mikechu-optimizely mikechu-optimizely deleted the mike/aat-default-vuid-use branch January 16, 2024 22:35
const { optimizely, userId, userAttributes, user } = props;
}

componentDidMount(): void {
Copy link

Choose a reason for hiding this comment

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

this introduces a bug and breaks SSR:
#234

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internal ticket opened and put into this sprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants