Fix issues with descendants and allDescendants
#3728
Conversation
| @@ -7,6 +7,10 @@ | |||
| * Enhanced `markdown` component to support the underlying `components` prop from | |||
| `react-markdown`. Use this prop to customize markdown rendering. | |||
|
|
|||
| ### 🐞 Bug Fixes | |||
| * The `Record.descendants` and `Record.allDescendants` getters were incorrectly returning the | |||
| record itself. This has been fixed. | |||
There was a problem hiding this comment.
Ah interesting - there is a chance there is some code out there that was depending on the prior behavior (intentionally or not), but hard to argue that it was correct. This looks like the right choice / fix to me.
There was a problem hiding this comment.
Agree that this change makes sense - previous behavior was definitely confusing - I vaguely remember this from when we were re-working these methods way back but can't remember why we included the record in the list of descendants
| children.forEach(child => this.gatherDescendantIds(child.id, idSet)); | ||
| this.childrenMap.get(id)?.forEach(child => { | ||
| if (!idSet.has(child.id)) { | ||
| // paranoia? did we encounter loops? |
There was a problem hiding this comment.
Looks like this came in a while ago with a big refactor that introduced many of these recordset APIs - doesn't look like a response to a particular bug or use case.
https://github.com/xh/hoist-react/pull/1082/files
(FWIW - I'd still vote to carry it forward - will cop to a certain amount of paranoia in these classes myself)
| @@ -7,6 +7,10 @@ | |||
| * Enhanced `markdown` component to support the underlying `components` prop from | |||
| `react-markdown`. Use this prop to customize markdown rendering. | |||
|
|
|||
| ### 🐞 Bug Fixes | |||
| * The `Record.descendants` and `Record.allDescendants` getters were incorrectly returning the | |||
| record itself. This has been fixed. | |||
There was a problem hiding this comment.
Agree that this change makes sense - previous behavior was definitely confusing - I vaguely remember this from when we were re-working these methods way back but can't remember why we included the record in the list of descendants
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.