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

Add a new throwPlayFunctionExceptions parameter #19143

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Sep 8, 2022

Issue: Play function errors were swallowed if you didn't have the interactions addon installed.

What I did

  • Add parameters.hidePlayFunctionExceptions
  • If unset, revert to throwing play function errors
  • Set it in the interactions addon

How to test

  • See unit tests
  • Run a sandbox, throw an error in a play function. Check it is still swallowed.
  • Disable the interactions addon, check the play function is displayed:

image

How to set in external tools (e.g. Chromatic)

  1. Browse to iframe.html.
  2. Update project parameters:
const preview = __STORYBOOK_PREVIEW__
const { projectAnnotations } = preview.storyStore;
const newAnnotations = {
  ...projectAnnotations,
  parameters: {
    ...projectAnnotations.parameters, 
    throwPlayFunctionExceptions: false,
  },
};

preview.onGetProjectAnnotationsChanged({ getProjectAnnotations: () => newAnnotations })
  1. Render story as usual.

If true, it doesn't exhibit play function exceptions in the UI. Set it to true from the interactions addon.
@tmeasday
Copy link
Member Author

tmeasday commented Sep 8, 2022

@ghengeveld one thing: in this implementation, the sequence of events is changed if parameters.hidePlayFunctionExceptions is set:

  1. With it set, we only throw PLAY_FUNCTION_THREW_EXCEPTION, the story ends up in phase "completed"
  2. With it unset, we throw PLAY_FUNCTION_THREW_EXCEPTION, then STORY_THREW_EXCEPTION and the story ends up in phase "errored".

I'm thinking in case 2, instead we probably want to show the error in the UI, but still end up completing the story without throwing a STORY_THREW_EXCEPTION. WDYT?

@yannbf
Copy link
Member

yannbf commented Sep 9, 2022

Question: why is the config set as a parameter (which could be changed at a story level for instance) and not as a core or features property from main.js/presets?

@tmeasday
Copy link
Member Author

tmeasday commented Sep 9, 2022

Question: why is the config set as a parameter (which could be changed at a story level for instance) and not as a core or features property from main.js/presets?

Good question! I don't think it's a feature, but a core property might make sense. I guess the main reason to use a parameter is to allow Chromatic to change it easily, I'm not sure how you'd change a core property from outside the SB build (maybe you can just update window.X directly?)

@tmeasday
Copy link
Member Author

Discussed with @shilman. In the future we might want to consider a proper "options store" in the preview, and also mechanisms/APIs for changing options at runtime. For now, it isn't really safe to change options at runtime, so we have to use parameter for this. We already have some other parameters that are really options so I guess we aren't exactly making things "worse" 🤷

Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I'd like to see it renamed to throwPlayFunctionExceptions though.

@tmeasday tmeasday merged commit d25ca7b into next Sep 12, 2022
@tmeasday tmeasday deleted the tom/sb-722-render-play-function-exceptions-in-the branch September 12, 2022 11:12
@tmeasday tmeasday changed the title Add a new hidePlayFunctionExceptions parameter Add a new throwPlayFunctionExceptions parameter Sep 12, 2022
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.

3 participants