Skip to content

Svelte: remount when resetting args in controls#21659

Merged
JReinhold merged 9 commits intonextfrom
21470-bug-the-reset-controls-button-appears-to-be-resetting-the-value-back-to-undefined
Mar 22, 2023
Merged

Svelte: remount when resetting args in controls#21659
JReinhold merged 9 commits intonextfrom
21470-bug-the-reset-controls-button-appears-to-be-resetting-the-value-back-to-undefined

Conversation

@JReinhold
Copy link
Copy Markdown
Contributor

@JReinhold JReinhold commented Mar 17, 2023

Closes #21470

What I did

This PR makes the Svelte renderer force remount the story when a user resets all args in the controls panel.

It does this by listening for the RESET_STORY_ARGS event, saves the information that a story is being reset, and uses that information in renderToCanvas to remount instead of just updating props.

There's a possible race condition here, because the RESET_STORY_ARGS event will both trigger this custom listener and also trigger renderToCanvas. If for some reason renderToCanvas reacts to the event before the custom listener, then it won't know that it should remount, and it will most likely remount on the following render, eg. triggered by a regular arg change.
I've been unable to produce this out-of-order triggering so for now it's purely theoretical.

There's still an issue here though, that when any singular control gets unset, that will not cause a full remount and the component will not revert to default but rather act as if the value was undefined. From a purely Svelte-rendering perspective this is expected, the same thing happens if your Svelte components do that (set a prop to something and then sets it to undefined). But maybe from a Storybook user perspective this is unexpected, at least I would expect it to go back to the default.
This scenario is only valid for the color control as it is the only control that can be unset. But I suspect we'll make this a universal feature in the future.

How to test

  1. See new test story being successful: https://630873996e4e3557791c069c-eagyhguotu.chromatic.com/?path=/story/stories-renderers-svelte-args--remount-on-reset-story-args
  2. Try it out with the default Button story:
    1. Open the Button story in a Svelte project
    2. Change the "size" control to "large"
    3. Reset args
    4. See that it goes back to the default
    5. Do the same thing but on the next branch and see that the button loses its size completely because size becomes undefined.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@JReinhold JReinhold marked this pull request as ready for review March 17, 2023 21:20
* We listen for the RESET_STORY_ARGS event and store the storyId to be reset
* We then use this in the renderToCanvas function to force remount the story
*/
const storyIdsToRemountFromResetArgsEvent = new Set<string>();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if this Set would ever have more than one entry at a time. It would only occur if RESET_STORY_ARGS were sent for multiple stories at the same time, and their renderToCanvas were all called after them (because renderToCanvas immediately removes the story id from the Set again).

I can't think of a scenario where that would happen - in that sense this could be simplified to be a single string instead of a Set.

However this feels more bulletproof in a way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I agree with all that 💯

@JReinhold JReinhold requested a review from tmeasday March 17, 2023 21:28
@JReinhold JReinhold self-assigned this Mar 17, 2023
@JReinhold JReinhold added the ci:daily Run the CI jobs that normally run in the daily job. label Mar 17, 2023
await within(canvasElement).findByText(/New Text/);
await expect(button).toHaveFocus();

await channel.emit(RESET_STORY_ARGS, { storyId: id });
Copy link
Copy Markdown
Contributor Author

@JReinhold JReinhold Mar 17, 2023

Choose a reason for hiding this comment

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

resetting args at the end of this test is unnecessary, because we recently added logic to reset args at the beginning also - both as a way to ensure that the story is in a clean state before executing.

Copy link
Copy Markdown
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM @JReinhold 👏

In 7.1 we should probably add information about whether this is a "reset" to the renderToCanvas call (IIRC that's the plan right?)

Do you have thoughts about the individual arg reset issue? Is this something that can realistically happen to a component? If not, should we just be resetting in the same way?

* We listen for the RESET_STORY_ARGS event and store the storyId to be reset
* We then use this in the renderToCanvas function to force remount the story
*/
const storyIdsToRemountFromResetArgsEvent = new Set<string>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I agree with all that 💯

@JReinhold
Copy link
Copy Markdown
Contributor Author

In 7.1 we should probably add information about whether this is a "reset" to the renderToCanvas call (IIRC that's the plan right?)

Yes, tracking in #21706

Do you have thoughts about the individual arg reset issue? Is this something that can realistically happen to a component? If not, should we just be resetting in the same way?

I actually think we should keep it as-is for now. Reseting an individual arg is synonymous with changing the prop to undefined (if the default is undefined for the story, ofc), and I think we should show the user what happens in reality when they do that. If we start remounting we're basically telling the user "if you change this prop to undefined it will go back to the default prop of the component", which it won't in their application, so this would be misleading.
I feel this is different from reseting all Controls to undefined, because that seems more like a Storybook scenario where you just want to go back to the initial state.

…g-the-reset-controls-button-appears-to-be-resetting-the-value-back-to-undefined
…g-the-reset-controls-button-appears-to-be-resetting-the-value-back-to-undefined
@JReinhold JReinhold merged commit 14cb942 into next Mar 22, 2023
@JReinhold JReinhold deleted the 21470-bug-the-reset-controls-button-appears-to-be-resetting-the-value-back-to-undefined branch March 22, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:daily Run the CI jobs that normally run in the daily job. svelte

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: The reset controls button appears to be resetting the value back to undefined

2 participants