react icon indicating copy to clipboard operation
react copied to clipboard

Allow non-memoized item lists

Open ipc103 opened this issue 1 year ago • 13 comments

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)

ipc103 avatar Feb 28 '24 19:02 ipc103

🦋 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

changeset-bot[bot] avatar Feb 28 '24 19:02 changeset-bot[bot]

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 avatar Feb 29 '24 15:02 siddharthkp

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

siddharthkp avatar Feb 29 '24 16:02 siddharthkp

Thanks @siddharthkp! Let me know if/how I can help.

ipc103 avatar Feb 29 '24 17:02 ipc103

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? 🤔

siddharthkp avatar Mar 01 '24 11:03 siddharthkp

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!

ipc103 avatar Mar 01 '24 15:03 ipc103

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.

ipc103 avatar Mar 01 '24 20:03 ipc103

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!

ajhenry avatar Mar 05 '24 20:03 ajhenry

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!

siddharthkp avatar Mar 22 '24 15:03 siddharthkp

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.

ipc103 avatar Mar 22 '24 15:03 ipc103

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.

github-actions[bot] avatar May 21 '24 16:05 github-actions[bot]

@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?

ipc103 avatar May 22 '24 02:05 ipc103

@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 🤞

ipc103 avatar May 22 '24 18:05 ipc103

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.

github-actions[bot] avatar Jul 21 '24 19:07 github-actions[bot]