Allow non-memoized item lists
Closes https://github.com/primer/react/issues/4315
CC @ajhenry
Changelog
Fixes a bug in the SelectPanel component which required the same object to be stored in the items and selectedItems lists.
New
N/A
Changed
Fixes a bug where SelectPanel wouldn't update state correctly when the items list contained different objects on re-render.
Removed
N/A
Rollout strategy
- [x] Patch release
- [ ] Minor release
- [ ] Major release; if selected, include a written rollout or migration plan
- [ ] None; if selected, include a brief description as to why
Testing & Reviewing
See the updated test cases for a minimal repro example.
Merge checklist
- [x] Added/updated tests
- [ ] Added/updated documentation
- [ ] Added/updated previews (Storybook)
- [x] Changes are SSR compatible
- [ ] Tested in Chrome
- [ ] Tested in Firefox
- [ ] Tested in Safari
- [ ] Tested in Edge
- [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)
🦋 Changeset detected
Latest commit: b4253e93f4243728e95d2bd38ca69b46d88344c2
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @primer/react | Patch |
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
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
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 @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.