Fix returning empty array in getEntityRecords for unknown entities #36984
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
kevin940726
added
[Type] Bug
kevin940726
requested review from
youknowriad,
noisysocks,
talldan,
gziolo and
aduth
|
Size Change: +391 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
kevin940726
commented
Nov 30, 2021
| @@ -160,14 +160,14 @@ describe( 'hasEntityRecords', () => { | |||
| expect( hasEntityRecords( state, 'root', 'postType' ) ).toBe( false ); | |||
| } ); | |||
|
|
|||
| it( 'returns true if the entity configuration is not known', () => { | |||
| it( 'returns false if the entity configuration is not known', () => { | |||
There was a problem hiding this comment.
This doesn't make sense to me either. Why would we want to return true for hasEntityRecords when the entity is still unknown?
There was a problem hiding this comment.
Yeah, agree, that doesn't seem right given the documentation for hasEntityRecords:
Returns true if records have been received for the given set of parameters, or false otherwise.
The key word being 'received'. That queried data prepopulates for known entity configurations is irrelevant, those records weren't received in the typical way we use that term.
I also find the name hasEntityRecords confusing since it should really be called hasReceivedEntityRecords. The current name would lead me to believe an empty array would return false too, but the function definition instead just checks Array.isArray.
And with the way it works in trunk it should perhaps just be called hasEntityConfiguration 😄
|
@kevin940726 It'd be good to detail what led you to making this fix. Does it fix a known bug or is it an observation you made about some code that could be improved? |
Sure! I found this bug while I was working on the templates list page. Specifically, I was calling I put a Since that the initial data is always |
|
This makes sense, and semantically I prefer it, but I'm not sure of what unexpected consequences it could have. So I'd defer to @youknowriad. (Also cc @ntsekouras — no action needed, just thought you might be interested in the topic.) |
Neither. I'd say let's document the change in |
youknowriad
approved these changes
Dec 1, 2021
adamziel
approved these changes
Dec 1, 2021
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
getEntityRecordscurrently will return an empty array if the entity is unknown (not fetched), but it will also later returnnullwhen the entity is loaded (viagetQueriedItems), until finally return the list of records when the data is fully fetched.This behavior doesn't make sense to me, I think we should also return
nullif the entity is unknown, to align with thenullvalue when the data is still fetching. By returningnullfor the initial request, the end developers can also better distinguish between a loaded empty list and a loading state.There are some former discussion about this: #19498 (comment), #21630 (comment). I believe the consensus was to always return
null.This shouldn't be a breaking change since it already returns
nullin other cases, and we could treat the original behavior as a bug.How has this been tested?
Unit tests should pass.
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).