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

UserProvider and useUser #4557

Merged
merged 5 commits into from
May 10, 2022
Merged

UserProvider and useUser #4557

merged 5 commits into from
May 10, 2022

Conversation

takameyer
Copy link
Contributor

@takameyer takameyer commented May 5, 2022

What, How & Why?

Add UserProvider and useUser hook to allow access to the user within any place in the application.

Note: There are no tests, as this is unreasonable to test without actually spinning up a BAAS server. This is an unrealistic expectation for this work and should be done in a greater effort to refactor tests. I have, however, throughly tested this with the example application. If we do start getting bugs, we should find a way to create regression tests.

We will also need a follow up PR to update the Realm dependency as soon as the new release is out, as this is dependent on the new App and User listener events.

This closes #4456

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@cla-bot cla-bot bot added the cla: yes label May 5, 2022
@takameyer takameyer marked this pull request as ready for review May 6, 2022 13:15
Comment on lines 94 to 97
const combinedConfigWithUser =
user && !combinedConfig.sync?.user
? mergeRealmConfiguration(combinedConfig, { sync: { user } })
: combinedConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Would this simplification be possible?

Suggested change
const combinedConfigWithUser =
user && !combinedConfig.sync?.user
? mergeRealmConfiguration(combinedConfig, { sync: { user } })
: combinedConfig;
const combinedConfigWithUser = mergeRealmConfiguration({ sync: { user } }, combinedConfig);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this suggestion, it is possible for a non-sync application to have {sync: {user: undefined}} included in the configuration. But maybe there is a way to simplify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

 const combinedConfigWithUser =
        combinedConfig?.sync && user ? mergeRealmConfiguration({ sync: { user } }, combinedConfig) : combinedConfig;

This is a little simpler.

Copy link
Member

@kraenhansen kraenhansen May 10, 2022

Choose a reason for hiding this comment

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

it is possible for a non-sync application to have {sync: {user: undefined}}

Yeah, I figured this could be an edge-case and wondered if mergeRealmConfiguration implemented a deletion of the sync property if all members has undefined values or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's perhaps another possibility

@takameyer takameyer merged commit 6ac2cbd into master May 10, 2022
@takameyer takameyer deleted the andrew/rr-use-user branch May 10, 2022 16:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create useApp and AppProvider
2 participants