(feat): Adds componentDidCatch() to top-level component #627

tizmagik · 2017-10-06T04:37:52Z

Adds componentDidCatch() to top-level <StyleGuide /> component. We could arguably have added an additional <ErrorBoundary /> layer in case there's an error in the <StyleGuide /> component itself, but that seemed unlikely.

To test this, you would need to update to React 16 locally (I left that out of this PR because there's still some other pending tasks in #614 ) and then access a property on an undefined object or something in the render method of a component deep in the tree. Here's a screenshot of what it looks like:

Note that the console also logs the React-generated error, which is nice.

codecov-io · 2017-10-06T04:37:54Z

Codecov Report

Merging #627 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
+ Coverage   96.01%   96.03%   +0.02%     
==========================================
  Files         100      101       +1     
  Lines        1330     1338       +8     
  Branches      270      271       +1     
==========================================
+ Hits         1277     1285       +8     
  Misses         51       51              
  Partials        2        2

Impacted Files	Coverage Δ
src/rsg-components/Error/ErrorRenderer.js	`100% <100%> (ø)`
src/rsg-components/StyleGuide/StyleGuide.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14346d6...a12867c. Read the comment docs.

tizmagik · 2017-10-06T04:38:40Z

src/rsg-components/StyleGuide/StyleGuide.js

+				<PlaygroundError
+					classes={{}}
+					message={
+						<div style={{ margin: 10 }}>


Wasn't sure how best to style, happy to change this, but it seemed fairly innocuous enough here 😄

I think we shouldn’t use PlaygroundError component here — full screen of ~~blood~~ would be an overkill, so just red text on white background would be enough. I think we can extract the layout from Welcome component to a Page (don’t know a better name for that) component and use it to show an error. Also please take a look at our developer guide: https://react-styleguidist.js.org/docs/development.html

Yea I wanted to minimize how much code is used when rendering an error; I even hesitated to use PlaygroundError since it could also contain errors itself. Do you think just straight text would work? I'd rather not introduce another component and avoid another potential failure point by doing so.

Let me push that up and we can talk through it more

tizmagik · 2017-10-06T04:39:19Z

src/rsg-components/PlaygroundError/PlaygroundErrorRenderer.js

@@ -21,7 +21,7 @@ export function PlaygroundErrorRenderer({ classes, message }) {

 PlaygroundErrorRenderer.propTypes = {
 	classes: PropTypes.object.isRequired,
-	message: PropTypes.string.isRequired,
+	message: PropTypes.oneOfType([PropTypes.string, PropTypes.node]).isRequired,


I changed the PropType here to also allow a Component to be passed in for better control over spacing

sapegin

Thanks for the pull request!!

sapegin · 2017-10-06T08:47:35Z

src/rsg-components/StyleGuide/StyleGuide.js

+				<PlaygroundError
+					classes={{}}
+					message={
+						<div style={{ margin: 10 }}>


I think we shouldn’t use PlaygroundError component here — full screen of ~~blood~~ would be an overkill, so just red text on white background would be enough. I think we can extract the layout from Welcome component to a Page (don’t know a better name for that) component and use it to show an error. Also please take a look at our developer guide: https://react-styleguidist.js.org/docs/development.html

sapegin · 2017-10-06T08:53:14Z

src/rsg-components/StyleGuide/StyleGuide.js

+							{`${this.state.error} ${this.state.info.componentStack}`}
+							<p>
+								This may be due to an error in a component you are overriding, or a bug in
+								react-styleguidist.<br />If you believe this is a bug, please submit an issue:&nbsp;


It is React Styleguidist ;-)

I’d make [submit an issue] a link, no need to show the URL in the UI.

And you can use Markdown component to render text, see WelcomeRenderer component.

sapegin · 2017-10-06T08:55:42Z

And we’ll need some new tests ;-)

sapegin

I understand your point of using less components but the problem of this approach is that it could be styled unpredictably. Easily it can be unreadable like white text on white background.

sapegin · 2017-10-06T14:08:26Z

src/rsg-components/StyleGuide/StyleGuide.js

-						</div>
-					}
-				/>
+				<div style={{ margin: 10, color: 'crimson' }}>


We still need to extract this — non *Render components shouldn’t print any HTML ;-)

Margin should be 8 or better 16 (we use 8px grid by default).

Heh okay if you insist. I can create a new overridable Error component to render the error message, but IMO that introduces an increased chance of situations where it's possible to hit an error condition while trying to render the error message itself. I guess we always have the console.error to fallback to? 😁

I’m not insisting on using JSS, etc. but I’m trying to explain why it’s not that easy as it look ;-)

So is it a matter of also setting a background color here? I'm not sure what approach you'd like to take here...

Yeah, I think that would be enough to ensure readability. I still hope it will be a rare case to see this thing in real life ;-)

Okay cool, I could do that 👍

Yea, hopefully this never even hits, but you never know 🤷‍♂️

I also think it’s safe to import styles from https://github.com/styleguidist/react-styleguidist/blob/master/src/styles/theme.js because these are just const values. They won’t be overridable by the use but we’ll have less hardcoded styles.

Yes, agreed, I actually started doing that, will push that up shortly

sapegin · 2017-10-06T14:09:07Z

src/rsg-components/StyleGuide/StyleGuide.js

-					}
-				/>
+				<div style={{ margin: 10, color: 'crimson' }}>
+					<code style={{ whiteSpace: 'pre' }}>{`${this.state.error} ${this.state.info


Any reason not to user <pre> tag? ;-)

I don’t think you need to wrap error message into another string.

<pre> makes sense 😁

Error needs to be wrapped because it's actually an object (stack trace), so this coerces it to a string.

Then I’d to .toString() to clearly show the intent.

sapegin · 2017-10-06T14:09:46Z

src/rsg-components/StyleGuide/StyleGuide.js

+						.componentStack}`}</code>
+					<p>
+						This may be due to an error in a component you are overriding, or a bug in React
+						Styleguidist.<br />If you believe this is a bug,&nbsp;


Probably would be better to print this in another paragraph.

tizmagik · 2017-10-06T15:16:56Z

Okay pushed up the changes we talked about. I'll add tests once we agree on the approach 👍

tizmagik · 2017-10-06T15:19:45Z

src/rsg-components/StyleGuide/StyleGuide.js

+							</a>.
+						</p>
+					</div>
+				</div>


We can move this "component" to a separate file, but I don't think it should be overridable by the user.

We can’t enforce that but it’s not a reason to move it ;-) Consistency is one reason and it’s just a bit chunk of HTML that I’d like to hide from this component.

Now I like it! 🍕

tizmagik · 2017-10-06T16:15:26Z

src/rsg-components/StyleGuide/StyleGuide.spec.js

+	const wrapper = shallow(<StyleGuide codeRevision={1} config={config} sections={[]} slots={{}} />);
+	wrapper
+		.instance()
+		.componentDidCatch({ toString: () => 'error' }, { componentStack: { toString: () => 'info' } });


Note the slight bit of complexity introduced here by explicitly using .toString() in the component 😞

Looks good to me ;-)

tizmagik · 2017-10-06T20:21:21Z

Good to merge?

sapegin · 2017-10-07T07:35:18Z

As soon as we extract the HTML to a separate component ;-)

tizmagik · 2017-10-07T13:13:03Z

Oh, not sure what you mean. I suggested a separate file but you said you liked it as it was? I even got a 🍕 emoji! Haha

Could you clarify, or better yet, commit to the branch 😁

sapegin · 2017-10-07T14:05:49Z

I like the code but I still want it in a separate file ;-)

sapegin · 2017-10-07T18:12:31Z

Merged, thanks!

(feat): Adds componentDidCatch() to top-level component

3921988

tizmagik commented Oct 6, 2017

View reviewed changes

inherit color

99e7d00

sapegin mentioned this pull request Oct 6, 2017

React 16 #614

Closed

17 tasks

sapegin requested changes Oct 6, 2017

View reviewed changes

Use less code

ca3cd28

sapegin reviewed Oct 6, 2017

View reviewed changes

Use theme

a0b2c4a

tizmagik commented Oct 6, 2017

View reviewed changes

Test

819f63f

tizmagik commented Oct 6, 2017

View reviewed changes

snap

4d4c9bc

More code

a12867c

sapegin approved these changes Oct 7, 2017

View reviewed changes

sapegin merged commit 5745d8b into master Oct 7, 2017

sapegin deleted the component-did-catch branch October 7, 2017 18:12

This was referenced Apr 7, 2021

[Snyk] Security upgrade css-loader from 0.28.7 to 1.0.0 Bhanditz/react-styleguidist#97

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#99

Open

This was referenced May 6, 2021

[Snyk] Security upgrade css-loader from 0.28.7 to 1.0.0 Bhanditz/react-styleguidist#106

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#108

Open

snyk-bot mentioned this pull request May 26, 2021

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#110

Open

snyk-bot mentioned this pull request May 26, 2021

[Snyk] Security upgrade css-loader from 0.28.7 to 1.0.0 Bhanditz/react-styleguidist#111

Open

This was referenced Sep 12, 2021

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#116

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#118

Open

Bhanditz mentioned this pull request May 16, 2022

[Snyk] Security upgrade css-loader from 0.28.7 to 1.0.0 Bhanditz/react-styleguidist#151

Open

snyk-bot mentioned this pull request May 17, 2022

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#153

Open

This was referenced Dec 19, 2023

[Snyk] Fix for 39 vulnerabilities Bhanditz/react-styleguidist#198

Open

[Snyk] Fix for 48 vulnerabilities Bhanditz/react-styleguidist#200

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat): Adds componentDidCatch() to top-level component #627

(feat): Adds componentDidCatch() to top-level component #627

tizmagik commented Oct 6, 2017 •

edited

Loading

codecov-io commented Oct 6, 2017 •

edited

Loading

tizmagik Oct 6, 2017

sapegin Oct 6, 2017

tizmagik Oct 6, 2017 •

edited

Loading

tizmagik Oct 6, 2017

tizmagik Oct 6, 2017

sapegin left a comment

sapegin Oct 6, 2017

sapegin Oct 6, 2017

sapegin commented Oct 6, 2017

sapegin left a comment

sapegin Oct 6, 2017

tizmagik Oct 6, 2017

sapegin Oct 6, 2017

tizmagik Oct 6, 2017

sapegin Oct 6, 2017

tizmagik Oct 6, 2017

sapegin Oct 6, 2017 •

edited

Loading

tizmagik Oct 6, 2017

sapegin Oct 6, 2017

tizmagik Oct 6, 2017

sapegin Oct 6, 2017

sapegin Oct 6, 2017

tizmagik commented Oct 6, 2017

tizmagik Oct 6, 2017

sapegin Oct 6, 2017

sapegin Oct 6, 2017

tizmagik Oct 6, 2017 •

edited

Loading

sapegin Oct 6, 2017

tizmagik commented Oct 6, 2017

sapegin commented Oct 7, 2017

tizmagik commented Oct 7, 2017

sapegin commented Oct 7, 2017

sapegin commented Oct 7, 2017

(feat): Adds componentDidCatch() to top-level component #627

(feat): Adds componentDidCatch() to top-level component #627

Conversation

tizmagik commented Oct 6, 2017 • edited Loading

codecov-io commented Oct 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tizmagik Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapegin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapegin commented Oct 6, 2017

sapegin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapegin Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tizmagik commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tizmagik Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tizmagik commented Oct 6, 2017

sapegin commented Oct 7, 2017

tizmagik commented Oct 7, 2017

sapegin commented Oct 7, 2017

sapegin commented Oct 7, 2017

tizmagik commented Oct 6, 2017 •

edited

Loading

codecov-io commented Oct 6, 2017 •

edited

Loading

tizmagik Oct 6, 2017 •

edited

Loading

sapegin Oct 6, 2017 •

edited

Loading

tizmagik Oct 6, 2017 •

edited

Loading