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

Core: Error on explicit actions while rendering or playing #25238

Merged
merged 32 commits into from
Jan 4, 2024

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Dec 15, 2023

What I did

  • Previously we transformed the type of action args (void functions) to be optional, as a default value is injected based on a regex. For 8.0 we want to go to a more explicit direction, as it is impossible to filter them out correctly based on a regex. If action args are required for rendering, they should be explicitly added to the args. I removed the logic that turned implicit actions into spies. If an arg is needed in the play function as spy, it should also be explicitly added.
  • The play function fails now when any error is thrown during the play function, even if it is thrown in the play function itself. This behavior is mimicked by how vitest works, and I also used the same flag dangerouslyIgnoreUnhandledErrors to disable this behavior:
    image
  • The above is necessary, to make sure that action can not be used during rendering or playing of the function. We now properly error out when someone runs the event during rendering or playing. This change is neccesary to make sure that tests behave the same if storybook is build with or without the --test flag.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-25238-sha-829df7a0. Install it by pinning all your Storybook dependencies to that version.

More information
Published version 0.0.0-pr-25238-sha-829df7a0
Triggered by @yannbf
Repository storybookjs/storybook
Branch kasper/remove-ts-magic
Commit 829df7a0
Datetime Fri Dec 29 17:55:22 UTC 2023 (1703872522)
Workflow run 7358741213

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=25238

Copy link
Contributor

github-actions bot commented Dec 15, 2023

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against d4efd04

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.

Does this require MIGRATION.md changes?

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.

I'm surprised we don't have to remove any tests as a result of this :/ Maybe we should add some tests that at least exercise the new way of doing it and check it works? (Could be separate to this PR of course)

@kasperpeulen kasperpeulen force-pushed the kasper/remove-ts-magic branch from b14913f to 146cd79 Compare December 22, 2023 15:45
@kasperpeulen kasperpeulen force-pushed the kasper/remove-ts-magic branch from 0758dd5 to 294302c Compare December 29, 2023 10:20
@kasperpeulen kasperpeulen marked this pull request as ready for review December 29, 2023 10:51
@yannbf
Copy link
Member

yannbf commented Dec 29, 2023

Overall this looks good! But we should definitely test this properly via a canary. I'm triggering a release now so we can test next week!

@kasperpeulen kasperpeulen merged commit 9ff64bc into next Jan 4, 2024
96 of 100 checks passed
@kasperpeulen kasperpeulen deleted the kasper/remove-ts-magic branch January 4, 2024 08:38
@github-actions github-actions bot mentioned this pull request Jan 4, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants