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

Interactions: Show exceptions by non-instrumented code in panel #16592

Merged
merged 27 commits into from
Jul 6, 2022

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 4, 2021

Issue: #16585

What I did

Catch exceptions thrown in the play function so they don't trigger a redbox.
Log them to console and emit an event if this happens and pick it up in the Interactions addon.
Show the error in the addon panel (see screenshot).
Fixed serialization of error payload in STORY_THREW_EXCEPTION event.

Screen Shot 2022-06-22 at 17 05 51

How to test

  • Is this testable with Jest or Chromatic screenshots? yes
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? no

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Nov 4, 2021

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 9be5035. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ghengeveld ghengeveld changed the title Show exceptions by non-instrumented code in the Interactions panel Interactions: Show exceptions by non-instrumented code in panel Nov 4, 2021
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 think we are missing tests here:

  1. Does the Panel have tests?
  2. We definitely should have a story that shows this new Exception component.
  3. Can we add a PreviewWeb test for the case that the play function throws? Or is this already covered?

lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
@shilman
Copy link
Member

shilman commented Jan 3, 2022

@tmeasday @ghengeveld what do we need to do to get this over the line?

@yannbf
Copy link
Member

yannbf commented Jun 16, 2022

Hey @ghengeveld, given the stuff you did in the other PRs, is this something we should work on still?

@ghengeveld
Copy link
Member Author

Hey @ghengeveld, given the stuff you did in the other PRs, is this something we should work on still?

I totally forgot about this one. I guess it would've been an alternative to adding any interactions that have an exception to the log. That is a nicer solution though so we don't need this PR for that purpose anymore. However there's also things which aren't instrumented that might throw an error. This PR does address those things, and I think it's much nicer to show those in the Interactions panel rather than a redbox.

@ghengeveld
Copy link
Member Author

Still needs some tests for the Panel and possibly PreviewWeb.

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 good with these changes as is, but have some suggestions for improvement maybe.

addons/interactions/src/Panel.tsx Outdated Show resolved Hide resolved
lib/core-server/src/utils/StoryIndexGenerator.ts Outdated Show resolved Hide resolved
lib/preview-web/src/StoryRender.ts Outdated Show resolved Hide resolved
@ghengeveld ghengeveld changed the base branch from next to future/base July 4, 2022 22:45
@ghengeveld
Copy link
Member Author

@shilman This is ready to merge.

@ghengeveld ghengeveld merged commit 968618c into future/base Jul 6, 2022
@ghengeveld ghengeveld deleted the 16585-exception-handling branch July 6, 2022 09:32
@shilman
Copy link
Member

shilman commented Aug 18, 2022

@ghengeveld @yannbf @tmeasday This is actually a breaking change, as we've seen today with the chromatic issue on PR #18894 and probably deserves its own entry in MIGRATION.md. WDYT?

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