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

Svelte: Fix docs rendering #19705

Merged
merged 13 commits into from
Nov 3, 2022
Merged

Svelte: Fix docs rendering #19705

merged 13 commits into from
Nov 3, 2022

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Nov 1, 2022

Issue: #19205

This PR fixes a number of issues:

  1. Only the last story in a docs file would be rendered when using the Svelte renderer. unblocks Disable play functions in docs mode, unless you set parameters.docs.autoplay #19659
  2. Changing controls would remount Svelte components instead of keeping them and their state
  3. A memory leak in the Vue3 renderer that uses a similar architecture.

Previously the Svelte renderer would destroy any previous component rendered whenever a new component was to be rendered. In docs with inline rendering this would cause each story to destroy each other immediately, leaving the last one remaining.

This PR (inspired by the vue3 renderer) instead keeps a map of all the parent DOM elements in use and only destroys them if the component to be rendered has a matching DOM element.

@JReinhold JReinhold self-assigned this Nov 1, 2022
@JReinhold JReinhold added maintenance User-facing maintenance tasks svelte vue3 labels Nov 1, 2022
@tmeasday
Copy link
Member

tmeasday commented Nov 1, 2022

As discussed with @ndelangen, a more bulletproof solution would be to have an unmount hook for renderers that is called whenever the user navigates away from a story, essentially telling the renderer "please release this component". Instead of the current situation where the following story is responsible for unmounting any existing stories.

Umm.... we do 🤷

export type TeardownRenderToRoot = () => Store_MaybePromise<void>;
export type RenderToRoot<TFramework extends AnyFramework, TStorybookRoot = HTMLElement> = (
context: Store_RenderContext<TFramework>,
element: TStorybookRoot
) => Store_MaybePromise<void | TeardownRenderToRoot>;

const teardown = await this.renderToScreen(renderContext, canvasElement);
this.teardownRender = teardown || (() => {});

return () => unmountElement(domElement);

(Sorry those code references are from this branch #19623 where I renamed renderToDOM to renderToRoot, but you get the idea).

@JReinhold
Copy link
Contributor Author

Good to know!
I'll try that out.

@JReinhold
Copy link
Contributor Author

I've reworked the implementation to

  1. use the teardown hook to teardown stories when appropriate
  2. fix the issue where any changes to props (via controls) would remount the whole component. Now it keeps the component state when changing controls.

2 should probably have some tests, but I think we should do that in a follow-up PR to unblock #19659 now. The tests would need a stateful component which none of our default components are - I think we should introduce that either way.

I'm pretty sure I can do the same remount fix for the Vue3 renderer in combination with the workarounds mentioned in vuejs/core#4874.

@JReinhold JReinhold marked this pull request as ready for review November 2, 2022 01:32
@shilman shilman added performance issue bug and removed maintenance User-facing maintenance tasks labels Nov 2, 2022
@tmeasday
Copy link
Member

tmeasday commented Nov 2, 2022

Nice job @JReinhold! Actually we already have a test for 2 -- it is just disabled for Svelte ATM:

['vue3', 'svelte', 'web-components', 'html', 'preact'].includes(globalThis.storybookRenderer)

As you can see I used focus as a stand-in for state ;) We could have also used the form component and filled it in a little bit.

@JReinhold
Copy link
Contributor Author

CI is blocking me on this.
E2E tests are failing because they are running on sandboxes that don't include the changes I've made to sandboxes in this PR.
(The Svelte examples weren't actually reactive, so the only reason the worked before was because we remounted them. now that we're not remounting, the sandboxes needs to be reactive.)

However I can't generate new repros for the repros repository, because CI is failing with ENOSPC: no space left on device. 🫠

@shilman shilman changed the title Fix Svelte rendering in Docs Svelte/Vue3: Fix docs rendering Nov 2, 2022
@JReinhold
Copy link
Contributor Author

my tests keeps failing because my changes to repros get overwritten by the scheduled repro generator, and thus the sandboxes wont update.

I think this is ready to merge when it has been reviewed @tmeasday . the tests passes locally, and the failing tests are exactly what I'd expect to see based on the changes I've made here.

code/renderers/svelte/src/render.ts Outdated Show resolved Hide resolved
code/renderers/svelte/src/render.ts Show resolved Hide resolved
@JReinhold JReinhold requested a review from tmeasday November 3, 2022 13:25
Copy link
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.

Svelte part looks great, maybe we should drop the vue part until we take another look at it?

code/renderers/vue3/src/render.ts Outdated Show resolved Hide resolved
@JReinhold
Copy link
Contributor Author

Svelte part looks great, maybe we should drop the vue part until we take another look at it?

Fine by me, this is a very minor fix anyway, that is incorporated in #19737 .

@JReinhold JReinhold removed the vue3 label Nov 3, 2022
@JReinhold JReinhold changed the title Svelte/Vue3: Fix docs rendering Svelte: Fix docs rendering Nov 3, 2022
@shilman shilman merged commit cc02a5c into next Nov 3, 2022
@shilman shilman deleted the jeppe/fix-svelte-in-docs branch November 3, 2022 23:53
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.

4 participants