Skip to content
This repository was archived by the owner on Jul 1, 2020. It is now read-only.

Building off of my other PR: Passing error to children #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Building off of my other PR: Passing error to children #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18ff009
Replacing div with React Fragment
krishnaglick May 20, 2020
3abbf75
making same change to server
krishnaglick May 20, 2020
a7d10dd
Adding snapshots and cleaning up tests
krishnaglick May 20, 2020
13df0ea
Passing error to children
krishnaglick May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/__snapshots__/client.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Client side Renders child component with context dependencies 1`] = `"<div>No errors! Context variable</div>"`;

exports[`Client side Renders child component with legacy context dependencies 1`] = `"<div>No errors! Context variable</div>"`;

exports[`Client side Renders child component with multiple contexts dependencies 1`] = `"<div>No errors! Context variable1 Context variable2 Context variable3</div>"`;

exports[`Client side Renders child component without errors 1`] = `"<div>No errors!</div>"`;

exports[`Client side Renders fallBack component if children rendering throws error 1`] = `"<div>FallBack!</div>"`;

exports[`Client side Renders fallBack component if children rendering throws error with contexts 1`] = `"<div>FallBack!</div>"`;

exports[`Client side Renders nothing when children rendering throws error and no fallBack provided 1`] = `null`;
11 changes: 11 additions & 0 deletions src/__snapshots__/server.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Server side Renders child component with context dependencies 1`] = `"<div><div>No errors! Context variable</div></div>"`;

exports[`Server side Renders child component with multiple new context dependencies 1`] = `"<div><div>No errors! Context variable1 Context variable2 Context variable3</div></div>"`;

exports[`Server side Renders child component without errors 1`] = `"<div><div>No errors!</div></div>"`;

exports[`Server side Renders fallBack component if children rendering throws error 1`] = `"<div>FallBack!</div>"`;

exports[`Server side Renders fallBack component if children rendering throws error with new contexts 1`] = `"<div>FallBack!</div>"`;
21 changes: 14 additions & 7 deletions src/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ describe('Client side', () => {
<GoodComponent />
</ErrorFallback>)

expect(component.html()).toBe('<div><div>No errors!</div></div>')
expect(component.html()).toBe('<div>No errors!</div>')
expect(component.html()).toMatchSnapshot()
})

it('Renders fallBack component if children rendering throws error', () => {
Expand All @@ -46,7 +47,8 @@ describe('Client side', () => {
<BadComponent />
</ErrorFallback>)

expect(component.html()).toBe('<div><div>FallBack!</div></div>')
expect(component.html()).toBe('<div>FallBack!</div>')
expect(component.html()).toMatchSnapshot()
turnOnErrors()
})

Expand All @@ -60,7 +62,8 @@ describe('Client side', () => {
<BadComponent />
</ErrorFallback>)

expect(component.html()).toBe('<div></div>')
expect(component.html()).toBe(null)
expect(component.html()).toMatchSnapshot()
turnOnErrors()
})

Expand Down Expand Up @@ -92,7 +95,8 @@ describe('Client side', () => {
</ContextProvider>
)

expect(component.html()).toBe('<div><div>No errors! Context variable</div></div>')
expect(component.html()).toBe('<div>No errors! Context variable</div>')
expect(component.html()).toMatchSnapshot()
})

it('Renders child component with context dependencies', () => {
Expand Down Expand Up @@ -122,7 +126,8 @@ describe('Client side', () => {
</ContextProvider>
)

expect(component.html()).toBe('<div><div>No errors! Context variable</div></div>')
expect(component.html()).toBe('<div>No errors! Context variable</div>')
expect(component.html()).toMatchSnapshot()
})

it('Renders child component with multiple contexts dependencies', () => {
Expand Down Expand Up @@ -168,7 +173,8 @@ describe('Client side', () => {
</ContextProvider>
)

expect(component.html()).toBe('<div><div>No errors! Context variable1 Context variable2 Context variable3</div></div>')
expect(component.html()).toBe('<div>No errors! Context variable1 Context variable2 Context variable3</div>')
expect(component.html()).toMatchSnapshot()
})

it('Renders fallBack component if children rendering throws error with contexts', () => {
Expand Down Expand Up @@ -203,7 +209,8 @@ describe('Client side', () => {
</ContextProvider>
)

expect(component.html()).toBe('<div><div>FallBack!</div></div>')
expect(component.html()).toBe('<div>FallBack!</div>')
expect(component.html()).toMatchSnapshot()
turnOnErrors()
})
})
21 changes: 17 additions & 4 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,31 @@ export function withContext (contextTypes = {}) {
}
static contextTypes = contextTypes
state = {
hasError: false
hasError: false,
error: null,
errorInfo: "",
}

componentDidCatch () {
componentDidCatch(error, errorInfo) {
this.setState({
hasError: true
hasError: true,
error,
errorInfo,
})
}

render () {
if (server) return server._render(this, ProvideContext)
return <div>{this.state.hasError ? this.props.fallBack() : this.props.children}</div>
const { error, errorInfo, hasError } = this.state;
if (hasError) {
return this.props.fallBack({ error, errorInfo });
} else {
Copy link

Choose a reason for hiding this comment

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

You can write conditions after "else" without "else". Possible, it'll be read better that way;)

const { fallBack, children } = this.props
if (hasError) return fallBack({ error, errorInfo })

return typeof children === 'function' ? children({ error, errorInfo }) : children

if (typeof this.props.children === "function") {
return this.props.children({ error, errorInfo });
} else {
return this.props.children;
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function _render (self, ProvideContext) {
const __html = renderToStaticMarkup(elementWithProviders)
return <div dangerouslySetInnerHTML={{__html}} />
} catch (e) {
return <div>{self.props.fallBack()}</div>
return <>{self.props.fallBack()}</>
Copy link

Choose a reason for hiding this comment

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

I think, the Fragmet syntax is excess here:) fallBack'll return some jsx elements already

Copy link

Choose a reason for hiding this comment

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

If the server's fallback gets an exception's error in arguments, it would be cool too;)

...
catch (error) {
  return self.props.fallBack({ error })
}

Copy link

Choose a reason for hiding this comment

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

This is going to be a problem when React tries to hydrate the DOM. If the DOM structure doesn't match what would normally be rendered, then it can result in zombie elements which do not get re-rendered. The fallback content isn't going to be the same as whatever we expected to render, but removing that div even with no fallback will be something that hydrate handles badly.

}
})
}
Expand Down
11 changes: 8 additions & 3 deletions src/server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import React from 'react'
import { renderToStaticMarkup } from 'react-dom/server'
import ErrorFallback, { withContext } from './index'
import { shim, clearContexts } from './server'
shim();
shim()

function FallBack () {
return <div>FallBack!</div>
Expand All @@ -32,6 +32,7 @@ describe('Server side', () => {
</ErrorFallback>)

expect(html).toBe('<div><div>No errors!</div></div>')
expect(html).toMatchSnapshot()
})

it('Renders fallBack component if children rendering throws error', () => {
Expand All @@ -44,7 +45,8 @@ describe('Server side', () => {
<BadComponent />
</ErrorFallback>)

expect(html).toBe('<div><div>FallBack!</div></div>')
expect(html).toBe('<div>FallBack!</div>')
expect(html).toMatchSnapshot()
turnOnErrors()
})

Expand Down Expand Up @@ -78,6 +80,7 @@ describe('Server side', () => {
)

expect(html).toBe('<div><div>No errors! Context variable</div></div>')
expect(html).toMatchSnapshot()
})

it('Renders child component with new context dependencies', () => {
Expand Down Expand Up @@ -155,6 +158,7 @@ describe('Server side', () => {
)

expect(html).toBe('<div><div>No errors! Context variable1 Context variable2 Context variable3</div></div>')
expect(html).toMatchSnapshot()
clearContexts()
})

Expand Down Expand Up @@ -190,7 +194,8 @@ describe('Server side', () => {
</ContextProvider>
)

expect(html).toBe('<div><div>FallBack!</div></div>')
expect(html).toBe('<div>FallBack!</div>')
expect(html).toMatchSnapshot()
turnOnErrors()
clearContexts()
})
Expand Down