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

Docs: Fix Source block snippet updates #22807

Merged

Conversation

chakAs3
Copy link
Contributor

@chakAs3 chakAs3 commented May 26, 2023

Closes #22679 #22645

What I did

Many users reported the same bug #22679.
in case the story has a boolean arg going back and forth between true/false. will not update the Source block.

previous code compares the newSource and current Source Object that contains all the sources using the stringified args as id, so if you toggle between 2 hashes that are already in the sources, you will not get any difference.

the fix is to compare the last source snippet with the new one

How to test

Run any sandbox the bug related to SourceContainer e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
2. Open Storybook in your browser
3. Access Any story

-->

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"]

@chakAs3 chakAs3 self-assigned this May 26, 2023
@chakAs3 chakAs3 changed the title Blocks(source-snippet):fix source snippet update issue Fix(Block:Source): source snippet update issue May 26, 2023
@tmeasday
Copy link
Member

tmeasday commented Jun 2, 2023

to be honest I don't see why we need to optimise the render here.

I'm not super concerned about optimizing, it's just that there's an entanglement between the Source block and the container that doesn't need to be there. It means the Source block depends on behaviour of the container that isn't necessarily "correct".

What that means is in the future, someone might come along and make a change to the container that makes sense in isolation (like optimizing, or something else?) and then breaks the Source block. It just seems like bad code hygiene for the block to depend on the container's reactivity to ensure its own works correctly.

I may have plugin that switches between two modes using context.parameters ( is the case for vue source decorator now)

I'm not sure I understand. The parameter can't change without changing story, right?

ndelangen and others added 2 commits June 2, 2023 09:11
Optimize webpack-builder: minify using swc
Migrate @storybook/web-components to strict TS
@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 2, 2023

to be honest I don't see why we need to optimise the render here.

I'm not super concerned about optimizing, it's just that there's an entanglement between the Source block and the container that doesn't need to be there. It means the Source block depends on behaviour of the container that isn't necessarily "correct".

What that means is in the future, someone might come along and make a change to the container that makes sense in isolation (like optimizing, or something else?) and then breaks the Source block. It just seems like bad code hygiene for the block to depend on the container's reactivity to ensure its own works correctly.

I may have plugin that switches between two modes using context.parameters ( is the case for vue source decorator now)

I'm not sure I understand. The parameter can't change without changing story, right?

I mean it is possible that I don't change args, but I still need to rerender the snippet, I want my source to include script setup tag for example, nothing to do with args or rendered story

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 2, 2023

if withScript === true

<script setup>
import  { ref } from 'vue"'
const  label = ref('Button')
const onClick = ()=>{};
<script>
<template>
  <Button :label="label" v-on:click="onClick" />
</template>

if withScript === false

<template>
  <Button label="Button" v-on:click="() => {}" />
</template>

These two snippets are for the same exact story.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 2, 2023

What that means is in the future, someone might come along and make a change to the container that makes sense in isolation (like optimizing, or something else?) and then breaks the Source block. It just seems like bad code hygiene for the block to depend on the container's reactivity to ensure its own works correctly.

100% Exact Container should not decide how Source Block behaves, should provide state whenever got changed. here it is true all blocks will get notified but that ok even if they rerender React handle the render optimisation by diffing, to make sure that the changed block will rerender properly to Real DOM

@tmeasday
Copy link
Member

tmeasday commented Jun 2, 2023

These two snippets are for the same exact story.

I don't get it. withScript is a parameter? You can't change parameters in a story -- so it can't be the same exact story.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 2, 2023

These two snippets are for the same exact story.

I don't get it. withScript is a parameter? You can't change parameters in a story -- so it can't be the same exact story.

yes but withScript does not impact the story render, only the source decorator, it is ok if you considered it not the same story, but Source Container use only the args as hashed id.

@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@storybook/preset-react-webpack@7.1.0-alpha.26 None +14 shilman
@storybook/core-server@7.1.0-alpha.26 None +42 shilman
@storybook/preset-server-webpack@7.1.0-alpha.26 None +10 shilman
@storybook/builder-webpack5@7.1.0-alpha.26 None +143 shilman
@storybook/builder-manager@7.1.0-alpha.26 None +3 shilman
@storybook/cli@7.1.0-alpha.26 None +24 shilman
@storybook/addon-docs@7.1.0-alpha.26 None +24 shilman
@storybook/core-common@7.1.0-alpha.26 None +24 shilman
@storybook/telemetry@7.1.0-alpha.26 None +6 shilman
@storybook/manager@7.1.0-alpha.26 None +0 shilman
@storybook/preview@7.1.0-alpha.26 None +0 shilman
@storybook/api@7.1.0-alpha.26 None +6 shilman
@storybook/client-api@7.1.0-alpha.26 None +0 shilman
@storybook/preview-web@7.1.0-alpha.26 None +0 shilman
@storybook/store@7.1.0-alpha.26 None +0 shilman
@storybook/addon-highlight@7.1.0-alpha.26 None +0 shilman
@storybook/addons@7.1.0-alpha.26 None +9 shilman
@storybook/server@7.1.0-alpha.26 None +4 shilman
@storybook/codemod@7.1.0-alpha.26 None +2 shilman
@storybook/core-client@7.1.0-alpha.26 None +0 shilman
@storybook/addon-controls@7.1.0-alpha.26 None +8 shilman
@storybook/csf-plugin@7.1.0-alpha.26 None +2 shilman
@storybook/router@7.1.0-alpha.26 None +0 shilman
@storybook/postinstall@7.1.0-alpha.26 None +0 shilman
@storybook/channel-websocket@7.1.0-alpha.26 None +0 shilman
@storybook/addon-toolbars@7.1.0-alpha.26 None +4 shilman
@storybook/node-logger@7.1.0-alpha.26 None +0 shilman
@storybook/theming@7.1.0-alpha.26 None +0 shilman
@storybook/addon-actions@7.1.0-alpha.26 None +12 shilman
@storybook/components@7.1.0-alpha.26 None +15 shilman
@storybook/addon-backgrounds@7.1.0-alpha.26 None +4 shilman
@storybook/react@7.1.0-alpha.26 None +20 shilman
@storybook/addon-viewport@7.1.0-alpha.26 None +4 shilman
@storybook/source-loader@7.1.0-alpha.26 None +0 shilman
@storybook/csf-tools@7.1.0-alpha.26 None +0 shilman
@storybook/addon-measure@7.1.0-alpha.26 None +4 shilman
@storybook/core-webpack@7.1.0-alpha.26 None +22 shilman
@storybook/manager-api@7.1.0-alpha.26 None +40 shilman
@storybook/blocks@7.1.0-alpha.26 None +14 shilman
@storybook/docs-tools@7.1.0-alpha.26 None +24 shilman
@storybook/addon-outline@7.1.0-alpha.26 None +4 shilman
@storybook/react-dom-shim@7.1.0-alpha.26 None +0 shilman
⬆️ Updated Package Version Diff Added Capability Access +/- Transitive Count Publisher
@storybook/preset-web-components-webpack@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +3/-0 shilman
@storybook/preset-vue-webpack@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +4/-0 shilman
@storybook/preset-vue3-webpack@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +4/-0 shilman
@storybook/preset-preact-webpack@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +3/-0 shilman
@storybook/preset-svelte-webpack@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +3/-0 shilman
@storybook/ember@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-21 shilman
@storybook/builder-vite@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +48/-0 shilman
@storybook/preset-html-webpack@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +3/-0 shilman
@playwright/test@1.34.3 1.32.3...1.34.3 None +1/-1 aslushnikov
storybook@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-60 shilman
@storybook/server-webpack5@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-28 shilman
@storybook/addon-storysource@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-11 shilman
@storybook/addon-essentials@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-27 shilman
@storybook/svelte-vite@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +10/-2 shilman
@storybook/addon-links@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-18 shilman
@storybook/react-webpack5@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-50 shilman
@storybook/addon-a11y@7.1.0-alpha.25 7.1.0-alpha.24...7.1.0-alpha.25 None +0/-22 shilman
@storybook/svelte@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +12/-0 shilman
@storybook/vue@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +12/-0 shilman
@storybook/vue3@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +8/-0 shilman
@storybook/addon-storyshots@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +4/-0 shilman
@storybook/preact@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +2/-0 shilman
@storybook/html@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +8/-0 shilman
@storybook/web-components@7.1.0-alpha.26 7.1.0-alpha.25...7.1.0-alpha.26 None +14/-0 shilman

🚮 Removed packages: @storybook/addon-actions@7.1.0-alpha.24, @storybook/addon-backgrounds@7.1.0-alpha.24, @storybook/addon-controls@7.1.0-alpha.24, @storybook/addon-docs@7.1.0-alpha.24, @storybook/addon-highlight@7.1.0-alpha.24, @storybook/addon-viewport@7.1.0-alpha.24, @storybook/addons@7.1.0-alpha.24, @storybook/blocks@7.1.0-alpha.24, @storybook/cli@7.1.0-alpha.24, @storybook/components@7.1.0-alpha.24, @storybook/instrumenter@7.1.0-alpha.25, @storybook/node-logger@7.1.0-alpha.24, @storybook/preview-web@7.1.0-alpha.24, @storybook/react@7.1.0-alpha.24, @storybook/server@7.1.0-alpha.24, @storybook/source-loader@7.1.0-alpha.24, @storybook/theming@7.1.0-alpha.24, playwright@1.32.3

@chakAs3 chakAs3 requested a review from shilman June 2, 2023 18:19
@chakAs3 chakAs3 merged commit 26f5d22 into chaks/add-e2e-test-to-addon-docs Jun 2, 2023
56 checks passed
@chakAs3 chakAs3 deleted the chaks/fix-update-source-snippet branch June 2, 2023 21:53
@tmeasday
Copy link
Member

tmeasday commented Jun 3, 2023

But what I am saying is if the parameter is different it will a different story, thus stored in the source container differently.

In either case the source container will update if it gets a new value for a given story/args' source so I'm not seeing the relevance.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 3, 2023

In either case the source container will update if it gets a new value for a given story/args'

Yes that is what it should be and what I did, I removed the optimisation test and just call setSources to update, and Each block will figure out what to render

@shilman
Copy link
Member

shilman commented Jun 5, 2023

@chakAs3 Jumping in late here. Looking through the conversation it looks like this PR re-renders all source snippets on the page every time there is a SNIPPET_RENDERED event, whereas @tmeasday is concerned that this fix should be located in the Source block since the block can access everything it needs to make that determination but is currently unaware of when the args change.

I looked the code a bit and it's not clear the best way to circumvent the rules of hooks with hacky workarounds, so I'd say let's go with @chakAs3 's fix for now. The rules of hooks thing will go away in Storybook 8.0 when we get rid of the deprecated code, so I'm going to add a comment to update things then.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 9, 2023

@chakAs3 Jumping in late here. Looking through the conversation it looks like this PR re-renders all source snippets on the page every time there is a SNIPPET_RENDERED event, whereas @tmeasday is concerned that this fix should be located in the Source block since the block can access everything it needs to make that determination but is currently unaware of when the args change.

I looked the code a bit and it's not clear the best way to circumvent the rules of hooks with hacky workarounds, so I'd say let's go with @chakAs3 's fix for now. The rules of hooks thing will go away in Storybook 8.0 when we get rid of the deprecated code, so I'm going to add a comment to update things then.

Excited for Storybook 8.0.

For me, the block should be only responsible for the snippet display behavior ( layout, style , actions: show/hide. , copy ..)
the optimization. should be done in the Decorator level, which is the case now for react and vue at least
we only fire SNIPPET_RENDERED if the args have changed

export const sourceDecorator = (storyFn: any, context: StoryContext<Renderer>) => {
  const skip = skipSourceRender(context);
  const story = storyFn();

  watch(
    () => context.args,
    () => {
      if (!skip) {
        generateSource(context);
      }
    },
    { immediate: true, deep: true }
  );
  return story;
};

so I really watching the changes, but again as you said the hooks implementation may mess with all this, as we need to call them every time, it is a temporary fix, but the work. will be more on the other side before. firing the event maybe

@JReinhold JReinhold added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs block: source bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Boolean control is not persistent