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

CSF: Expose story id in composeStories #22471

Merged
merged 6 commits into from
May 11, 2023

Conversation

sjwilczynski
Copy link
Contributor

@sjwilczynski sjwilczynski commented May 9, 2023

What I did

In this PR we expose story id when using composeStory/composeStories. We do that to be able to identify a story when running unit tests. You can check https://github.com/microsoft/nova-facade/pull/72/files#diff-220d974cd0e74b45dc745889f0752372189a35d53bc122ac00ec3f52edf7063f to see what it can be used for. In this case we have a decorator which for each story saves something in module state. To be able to interact with that entity inside play function we need to be able to identify a story somehow and such identifier can be used as a key for that store (and relying on id instead of manually setting the storyName). To be honest my preferred solution would be to just add the information from decorator to parameters (in case of that PR, to set environment as another parameter from within decorator). But as discussed in #21252 (comment), it seems that changes to parameters are not visible in play function and I am not sure how long it would take for the behavior to be fixed. Therefore, this PR is a workaround but IMO also provides a nicer way to setup context for play function from within unit tests.

@yannbf, @tmeasday I am looking forward to hear your thoughts on this!

How to test

Check the added unit tests, to see the exposed ids.

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

@sjwilczynski sjwilczynski marked this pull request as ready for review May 9, 2023 13:08
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

This looks great to me! Thank you for such detailed explanation of your use case 👍

code/lib/types/src/modules/composedStory.ts Outdated Show resolved Hide resolved
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

This looks great to me! Thank you for such detailed explanation of your use case 👍

@yannbf yannbf added bug patch:yes Bugfix & documentation PR that need to be picked to main branch csf labels May 10, 2023
@yannbf yannbf self-assigned this May 10, 2023
@sjwilczynski
Copy link
Contributor Author

sjwilczynski commented May 10, 2023

@yannbf thanks for the feedback, please let me know in case there is any action item on me, to get the PR in. Otherwise looking forward to it being merged and released!

@yannbf yannbf changed the title feat(csf-testing-utils): expose id of the story CSF: expose id of the story in composeStories testing utility May 11, 2023
@yannbf yannbf changed the title CSF: expose id of the story in composeStories testing utility CSF: expose story id in composeStories testing utility May 11, 2023
@yannbf yannbf merged commit 87ef5cf into storybookjs:next May 11, 2023
@sjwilczynski sjwilczynski deleted the stwilczy/idInComposeStories branch May 11, 2023 19:49
@shilman shilman added maintenance User-facing maintenance tasks and removed bug labels May 12, 2023
@shilman shilman changed the title CSF: expose story id in composeStories testing utility CSF: Expose story id in composeStories testing utility May 12, 2023
@shilman shilman changed the title CSF: Expose story id in composeStories testing utility CSF: Expose story id in composeStories May 12, 2023
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 12, 2023
shilman pushed a commit that referenced this pull request May 12, 2023
CSF: expose story id in composeStories testing utility
@shilman shilman mentioned this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csf maintenance User-facing maintenance tasks 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.

3 participants