-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[core-data] Do not suppress errors in the getEntityRecord and getEntityRecords resolvers #39317
[core-data] Do not suppress errors in the getEntityRecord and getEntityRecords resolvers #39317
Conversation
Size Change: +6 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
Is there any way to not reject but communicate the error and then in a future version reject? |
|
To be honest I wonder if we should just go ahead and make the change (remove the try/catch). Some breaking changes feel more like bug fixes (like here :) ) |
@youknowriad ha, that's how this PR started! :-) Then I chatted to @draganescu about deprecating it instead, but given that more people have the same reaction as I initially did, I reverted the deprecation notice and went for removing the catch clause entirely instead. Also ccing @gziolo for reviews. |
End to end tests seems to be failing in the same way as on my other PRs, must be a global issue – let's not block reviewing on that. |
I don't have enough context here to be helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great 👏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
What?
Now that the resolution errors are handled by our redux store, core-data resolvers may stop silencing them.
Note this is a breaking change in that the promises returned by resolveSelect( coreStore ).getEntityRecord/Records will now be rejected on failure whereas previously they were resolved without any possible way of learning that an error occurred.
Testing Instructions
Check if all the checks below this PR are green.
cc @kevin940726 @jsnajdr @youknowriad @gziolo @Mamaduka @draganescu