feat: add a logout function #595
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I've found this is more flexible and more idiomatic for components intended to be used in React applications (vs generic JSX applications, which imho this package should not strive to support)
this resets the store, which isn't exposed in the client's types. I think it's a little better to manage the store manually anyway since we're going to be calling a method on it directly, so I'm happy with this but could be dissuaded.
Previously it was possible (and common in the case where the user hit a page to log out) for store to be undefined, because setupClient had not yet run. This actively loads the store before calling reset, which is much more reliable.
gobengo
reviewed
Nov 30, 2023
| } | ||
|
|
||
| useEffect(() => { void getClient() }, []) // load client - once. | ||
| useEffect(() => { void setupClient() }, []) // load client - once. |
There was a problem hiding this comment.
if props.connection changes, wouldn't we want this to run again? so that a new client gets created and passed to setClient?
maybe setupClient assignment should use React.useCallback with appropriate dependencies, and this useEffect can depend on the variables it depends on e.g. setupClient
There was a problem hiding this comment.
also maybe we should have the lint rule for react hook deps? https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/README.md
There was a problem hiding this comment.
ah, I think you're right! I think that's orthogonal to the purpose of this PR though - happy to file an issue and even work on it today, but want to get this PR merged and shipped to fix the regression in production
gobengo
reviewed
Nov 30, 2023
| } | ||
|
|
||
| useEffect(() => { void getClient() }, []) // load client - once. | ||
| useEffect(() => { void setupClient() }, []) // load client - once. |
There was a problem hiding this comment.
Suggested change
| useEffect(() => { void setupClient() }, []) // load client - once. | |
| useEffect(() => { void setupClient() }, [servicePrincipal, connection, setupClient]) |
There was a problem hiding this comment.
hmmmm, not sure about this - on at the very least needs an update to the comment to explain why we're doing something so unusual, and I'm pretty sure this would trigger the linter you suggested above (though tbh I'm not a huge fan of that linter since it is not always correct and triggers on some valid use-cases)
gobengo
approved these changes
Nov 30, 2023
There was a problem hiding this comment.
consider this suggestion https://github.com/web3-storage/w3ui/pull/595/files#r1411076680
but IMO fine to address my comment in another issue
This was referenced Nov 30, 2023
travis
pushed a commit
that referenced
this pull request
Nov 30, 2023
🤖 I have created a release *beep* *boop* --- ## [1.1.0](core-v1.0.1...core-v1.1.0) (2023-11-30) ### Features * add a logout function ([#595](#595)) ([0995fd5](0995fd5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
travis
added a commit
that referenced
this pull request
Nov 30, 2023
🤖 I have created a release *beep* *boop* --- ## [1.1.0](react-v1.0.1...react-v1.1.0) (2023-11-30) ### Features * add a logout function ([#595](#595)) ([0995fd5](0995fd5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Travis Vachon <[email protected]>
travis
pushed a commit
that referenced
this pull request
Nov 30, 2023
🤖 I have created a release *beep* *boop* --- ## [1.2.0](react-v1.1.1...react-v1.2.0) (2023-11-30) ### Features * add a logout function ([#595](#595)) ([0995fd5](0995fd5)) * adds space-finder autocomplete combobox ([#268](#268)) ([3dcd647](3dcd647)) * allow users to set page size in W3APIProvider ([#308](#308)) ([814a293](814a293)) * club tropical w3 auth boxen ([#350](#350)) ([2266eb2](2266eb2)) * Customizable UI components ([#208](#208)) ([0a776fe](0a776fe)) * implement reverse paging ([#381](#381)) ([10f059a](10f059a)) * Improve upload component flow ([#285](#285)) ([ba9a3bf](ba9a3bf)) * simplify ([#591](#591)) ([d1dfdf0](d1dfdf0)) * Storybook story improvements ([#294](#294)) ([e0de2cc](e0de2cc)) ### Bug Fixes * fix w3console styling ([#320](#320)) ([74a298c](74a298c)) * homepage URL in package.json ([1229119](1229119)) * remove authenticator class when registed ([#352](#352)) ([3668f3b](3668f3b)) * w3console polish ([#284](#284)) ([9a67365](9a67365)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
travis
added a commit
to storacha/console
that referenced
this pull request
Dec 1, 2023
This will work once storacha/w3ui#595 is released.
travis
added a commit
that referenced
this pull request
Dec 4, 2023
One of the core goals here was to fix a bug where the client wouldn't get reset when servicePrincipal or connection changed. The easiest way to make sure this was tested was to extract all of the datamodel creation and management logic to a dedicated hook and test that. Thanks to @gobengo for pointing out the bug and suggesting the fix: #595 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resets the store, deleting the local agent's private keys and saved delegations. This is better than the previous implementation, and is appropriate for use cases like using console on a public or shared computer.
This PR also has a small tweak to replace JSX.Element with ReactNode, which I've found works better in downstream applications.