Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon-docs: Ensure we don't clobber multiple source container state updates #16039

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

tmeasday
Copy link
Member

Issue: As reported by @shilman

http://localhost:9011/?path=/docs/addons-docs-props--arg-types in Official Storybook

What I did

Ensure we don't clobber state if multiple events are received before a re-render finishes

How to test

  • Is this testable with Jest or Chromatic screenshots?

I'm not sure if we have the ability to test this outside of e2e?

@nx-cloud
Copy link

nx-cloud bot commented Sep 14, 2021

Nx Cloud Report

CI ran the following commands for commit dd0821c. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented Sep 14, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit dd0821c.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested! ✅

@tmeasday My only concern is that AFAIK setSources(current) will trigger a re-render, so this might result in extra renders?

@shilman shilman changed the title Ensure we don't clobber multiple source container state updates Addon-docs: Ensure we don't clobber multiple source container state updates Sep 15, 2021
@shilman shilman merged commit 008867d into next Sep 15, 2021
@shilman shilman deleted the fix/multiple-story-source-snippet branch September 15, 2021 00:13
@tmeasday
Copy link
Member Author

@shilman

(a) when we return current react will not re-render, that's part of the contract of useState.
(b) we actually re-render less than before, which is why this bug happened, on the plus side.

@tmeasday
Copy link
Member Author

PS the code doesn't quite make sense to me (why do we both check for equality on (newItem !== sources[id]) and using deepEqual?) but I tried to change it semantically as little as possible.

@shilman shilman added this to the 6.4 PRs milestone Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants