Core Data: Resolve getCurrentUser on API error #38989
Closed
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
Mamaduka
added
[Package] Core data
|
Would #38669 fix it as well? If we merge both, the error details will be lost. |
|
Size Change: +5 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Good point, @adamziel. I wondered if we should remove the current user from the store if the API request fails. Can we trust previously determined |
Oh that's a great question. There are two aspects that comes to mind:
I believe the backend authentication already takes care of the former, so we're only talking about the UI aspects. And in this case... it's quite a complex thing. For example, maybe the error is a temporary network issue? Then maybe we're still logged in as the same user. But perhaps it's an Unauthorized error? Then yes, it makes sense to remove the But if we're logged out, then most other requests would fail, too, so how do we handle that? By the way, we could make the same case for many resolvers. For example, if an entity record is being re-requested but turns out to be missing, it would make sense to remove it from the state and ensure it is somehow communicated to the user. But maybe it was just a packet gone missing and we're good to just silently retry the request? All in all, it sounds like a larger discussion to have about handling different ways in which the requests may fail in different contexts. cc @gziolo @jsnajdr @youknowriad |
|
Thanks, @adamziel. I agree this sounds like a part of the larger discussion. |
|
@adamziel, fantastic work on that PR 🦸 Do you have cases to use new meta selectors in the core? Then I can use it as an example to resolve this issue. |
|
@Mamaduka I think it haven't looked into how the selectors could be used across the Gutenberg yet, but there's one example from @getdave's that leans on the fact that resolveSelect throws an error: The #38669 will mark the resolver as "finalized", but there's still a matter of doing something with the error. I'm not too convinced about using |
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.
Description
See #37682.
Add
try/catchtogetCurrentUser. Even if we do nothing, catching an error ensures the resolver is marked as resolved.Testing Instructions
I couldn't figure out how to test this manually, so I included unit tests.
Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).