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

(fix) - Error boundary not applying in array cases #1572

Merged
merged 18 commits into from
Apr 26, 2019
Merged

(fix) - Error boundary not applying in array cases #1572

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
aa764de
fix: error boundary not applying in nested cases
JoviDeCroock Apr 24, 2019
f5db55e
fully remove _processing
JoviDeCroock Apr 24, 2019
0526fa0
try moving it
JoviDeCroock Apr 24, 2019
b57507f
cleanup code
JoviDeCroock Apr 24, 2019
843cd20
progress towards a working variant
JoviDeCroock Apr 25, 2019
41cbac6
Merge branch 'master' into fix/errorBoundary
JoviDeCroock Apr 25, 2019
55820e6
golf some bytes
JoviDeCroock Apr 25, 2019
6e0d8bb
add to mangle
JoviDeCroock Apr 25, 2019
d734e8f
make diff easier to read
JoviDeCroock Apr 25, 2019
a3f3767
byte golf
JoviDeCroock Apr 25, 2019
493689c
fix: mangle indentation
JoviDeCroock Apr 25, 2019
f428892
move clear
JoviDeCroock Apr 25, 2019
8b3bd50
update tests and golf bytes
JoviDeCroock Apr 25, 2019
c76941b
golf more bytes
JoviDeCroock Apr 25, 2019
b6ae8d2
reduce PR diff noise
JoviDeCroock Apr 25, 2019
6c1452c
more pr noise reduction (mixed tabs & spaces)
JoviDeCroock Apr 25, 2019
e4ddf7f
document _pendingError
JoviDeCroock Apr 25, 2019
cfeb001
mangle _pendingError to __E (adds 3B)
JoviDeCroock Apr 25, 2019
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
1 change: 1 addition & 0 deletions src/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export function Component(props, context) {
// this._nextState = null; // Only class components
// this._prevVNode = null;
// this._processingException = null; // Always read, set only when handling error
// this._pendingError = null; // Always read, set only when handling error. This is used to indicate at diffTime to set _processingException
}

/**
Expand Down
11 changes: 4 additions & 7 deletions src/diff/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,7 @@ export function diff(dom, parentDom, newVNode, oldVNode, context, isSvg, excessD
if (options.diff) options.diff(newVNode);

let c, p, isNew = false, oldProps, oldState, snapshot,
newType = newVNode.type;

/** @type {import('../internal').Component | null} */
let clearProcessingException;
newType = newVNode.type, clearProcessingException;

try {
outer: if (oldVNode.type===Fragment || newType===Fragment) {
Expand Down Expand Up @@ -74,7 +71,7 @@ export function diff(dom, parentDom, newVNode, oldVNode, context, isSvg, excessD
// Get component and set it to `c`
if (oldVNode._component) {
c = newVNode._component = oldVNode._component;
clearProcessingException = c._processingException;
clearProcessingException = c._processingException = c._pendingError;
dom = newVNode._dom = oldVNode._dom;
}
else {
Expand Down Expand Up @@ -188,7 +185,7 @@ export function diff(dom, parentDom, newVNode, oldVNode, context, isSvg, excessD
}

if (clearProcessingException) {
c._processingException = null;
c._pendingError = c._processingException = null;
}

if (options.diffed) options.diffed(newVNode);
Expand Down Expand Up @@ -382,7 +379,7 @@ function catchErrorInComponent(error, component) {
else {
continue;
}
return enqueueRender(component._processingException = component);
return enqueueRender(component._pendingError = component);
}
catch (e) {
error = e;
Expand Down
1 change: 1 addition & 0 deletions src/internal.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export interface Component<P = {}, S = {}> extends preact.Component<P, S> {
_prevVNode?: VNode | null;
_ancestorComponent?: Component<any, any>;
_processingException?: Component<any, any> | null;
_pendingError?: Component<any, any> | null;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We need to add this to our mangling properties to avoid potential name clashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it matter what name I give it? Was wondering that since I see clashing names in mangle and that worries me to pick one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mangling this to __E actually adds 3B

Copy link
Member

Choose a reason for hiding this comment

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

The default is to mangle _ properties to single-letter variables, so that makes sense. However, we want to avoid single-letter property names since they're likely to collide.

}

export interface PreactContext extends preact.Context<any> {
Expand Down
18 changes: 18 additions & 0 deletions test/browser/lifecycle.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2455,6 +2455,24 @@ describe('Lifecycle methods', () => {
expect(Receiver.getDerivedStateFromError).to.have.been.called;
});

// https://github.com/developit/preact/issues/1570
it('should handle double child throws', () => {
const Child = ({ i }) => {
throw new Error(`error! ${i}`);
};

const fn = () => render(
<Receiver>
{[1, 2].map(i => <Child key={i} i={i} />)}
</Receiver>,
scratch
);
expect(fn).to.not.throw();

rerender();
expect(scratch.innerHTML).to.equal('<div>Error: error! 2</div>');
});

it('should be called when child fails in componentWillMount', () => {
class ThrowErr extends Component {
componentWillMount() {
Expand Down