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.

Sign up for GitHub

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

Jump to bottom

Core: Show exception rather than error on react error boundary #8100

Merged
merged 2 commits into from
Oct 4, 2019
Merged

Core: Show exception rather than error on react error boundary #8100

merged 2 commits into from
Oct 4, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Sep 17, 2019 •
edited
Loading

Issue: #8087

What I did

(Follow-up to #8097)

Use showExecption (story exception) rather than showError (invalid object returned from story function) on React Error Boundary.

How to test

  • official-storybook
  • chromatic (on this PR)

@shilman shilman added bug high priority patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Sep 17, 2019
@shilman shilman added this to the 5.2.0 milestone Sep 17, 2019
@vercel
Copy link

vercel bot commented Sep 17, 2019 •
edited
Loading

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-8087-show-exception.storybook.now.sh

tmeasday
tmeasday approved these changes Sep 17, 2019
View reviewed 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.

This seems correct to me but would be interested in hearing from @Hypnosphi.

If the user code throws we want to do storyThrewException with a proper error object.

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 17, 2019 •
edited
Loading

Can you please take a screenshot of an example error in development? Does it contain duplicated stacktrace?

@Hypnosphi
Copy link
Member

chromatic (on this PR)

Not really, see #8087 (comment)

@shilman
Copy link
Member Author

shilman commented Sep 17, 2019

@Hypnosphi

Storybook

Hypnosphi
Hypnosphi reviewed Sep 17, 2019
View reviewed changes
app/react/src/client/preview/render.tsx
componentDidCatch({ message, stack }: Error) {
const { showError } = this.props;
componentDidCatch(err: Error) {
const { showException } = this.props;
// message partially duplicates stack, strip it
Copy link
Member

Choose a reason for hiding this comment

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

Let's actually do it again

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm will wait for @tmeasday 's feedback on this. Apparently it interacts with Chromatic in some way

Copy link
Member

@Hypnosphi Hypnosphi Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

chromatic can't test pages with errors anyway
#8087 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Please at least remove the comment if we're not actually stripping anything

Copy link
Member

Choose a reason for hiding this comment

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

@Hypnosphi FYI the issue here was that tools like Chromatic listen to to the storyThrewException event in order to show the user a stack trace of broken stories. This was reverting a regression where the wrong event was fired in that case.

Would be good to handle the distinction between "user code error" and "react error" somehow though.

@shilman
Copy link
Member Author

shilman commented Sep 17, 2019

Storybook

@shilman shilman mentioned this pull request Sep 17, 2019
Closed
@tmeasday
Copy link
Member

tmeasday commented Sep 18, 2019 •
edited
Loading

Can we distinguish the two types of errors in the componentDidCatch? Theoretically the first case (user code threw) should be passed unmodified to storyThrewException and the second as an object to storyErrored (confusing names, I know).

@shilman shilman modified the milestones: 5.2.0, 5.2.x Sep 23, 2019
@ndelangen
Copy link
Member

What's blocking this?

@shilman
Copy link
Member Author

shilman commented Sep 24, 2019

Can we distinguish the two types of errors in the componentDidCatch?

I don't know how to do this

@shilman shilman merged commit 6aef626 into next Oct 4, 2019
@shilman shilman deleted the 8087-show-exception branch October 4, 2019 07:11
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Oct 7, 2019
shilman added a commit that referenced this pull request Oct 7, 2019
@shilman
Core: Show exception rather than error on react error boundary (#8100)
aeda584 
Core: Show exception rather than error on react error boundary
This was referenced Oct 17, 2019
Merged
Merged
This was referenced Oct 17, 2019
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hypnosphi Hypnosphi Hypnosphi left review comments

@tmeasday tmeasday tmeasday approved these changes

@thomasbertet thomasbertet Awaiting requested review from thomasbertet

Assignees
No one assigned
Labels
bug core 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
Milestone
5.2.x
Development

Successfully merging this pull request may close these issues.
4 participants
@shilman @Hypnosphi @tmeasday @ndelangen