Allow non-memoized item lists #4324
Conversation
Towards primer#4315 The SelectPanel only did a basic equality check for the item state, meaning that it depended on having the exact same objects on multiple pass throughs. This isn't always possible as sometimes you may want to have different objects. This replaces the equality check with a test for an `id` property on the object. If the `id` property isn't present, we fallback to the old behavior. Note that a previous version used the `key` prop, but we decided `id` was a better interface. f472da2#r139163795 Co-authored-by: Andrew Henry <ajhenry@github.com>
🦋 Changeset detectedLatest commit: b4253e9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Towards primer#4315 The SelectPanel only did a basic equality check for the item state, meaning that it depended on having the exact same objects on multiple pass throughs. This isn't always possible as sometimes you may want to have different objects. This replaces the equality check with a test for an `id` property on the object. If the `id` property isn't present, we fallback to the old behavior. Note that a previous version used the `key` prop, but we decided `id` was a better interface. f472da2#r139163795 Co-authored-by: Andrew Henry <ajhenry@github.com>
| if (selectedItem.hasOwnProperty('id')) { | ||
| return selectedItem.id === item.id | ||
| } |
There was a problem hiding this comment.
I think we should also try testing for key since that's React's way of differentiating component changes
There was a problem hiding this comment.
I thought about that, but I actually think it's better to add the key when rendering over the list instead of trying to add it preemptively. Not super opinionated though if we want to use the key here!
|
Heads up: We can test if these changes are compatible with instances in gh/gh by creating an integration PR with this action: https://github.com/github/primer-engineering/blob/main/how-we-work/testing-primer-react-pr-at-dotcom.md |
siddharthkp
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Followup: Here's the integration PR https://github.com/github/github/pull/314966 but I'm expected it to fail today (false negatives!!) because there are a couple unmerged fixes with gh/gh 😓 I'll update this PR once the fixes are merged |
|
Thanks @siddharthkp! Let me know if/how I can help. |
|
Update: Here's the new integration PR: https://github.com/github/github/pull/315122 The CI is failing here. Is it because of the changes in this PR or unrelated? 🤔 |
|
It does seem like some of those are related to this PR. I wonder if there are spots where we're working around the current behavior, or otherwise depending on it. I'll take a look and see if I can make this a non-breaking change! |
|
Looking more closely I'm not convinced those failures are related. They seem to be about unrelated components. I'm going to re-run just to make sure they aren't flakes or something. |
|
Hey @siddharthkp anything we can help out with here? Looking through the test failures, it doesn't seem like this change should be involved but happy to debug where we can! |
Hey! sorry for the silence! Yeah, it's always tricky to tell when the integration tests have other errors as well 😢 If this is not urgent, I'm going to wait for the next release to go out so that I can have a clean slate to test this! Hope that's okay! |
|
Thanks @siddharthkp - that should work for us. The workaround is to just make sure that the list is memoized, so we can keep that up in the meantime. |
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
|
@siddharthkp 👋 Hi! What do you think about this one - any chance we could cut another integration test branch and see if this change plays more nicely with the newer release? |
|
@siddharthkp I went ahead and created a new integration PR: https://github.com/github/github/pull/325842 I used the latest release candidate since it seems like canary builds are on pause at the moment. Did I do it right? If so, I think everything is passing 🤞 |
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
Closes #4315
CC @ajhenry
Changelog
Fixes a bug in the
SelectPanelcomponent which required the same object to be stored in theitemsandselectedItemslists.New
N/A
Changed
Fixes a bug where
SelectPanelwouldn't update state correctly when theitemslist contained different objects on re-render.Removed
N/A
Rollout strategy
Testing & Reviewing
See the updated test cases for a minimal repro example.
Merge checklist