Skip to content

Removing undocumented BaseComponent.onError handler. #3956

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

Merged
dzearing merged 3 commits into from
Feb 14, 2018
Merged

Removing undocumented BaseComponent.onError handler. #3956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a67336f
Removes the default BaseComponent.onError handler and deprecates the …
dzearing Feb 13, 2018
c88c517
Adding change file.
dzearing Feb 13, 2018
4b5d842
Updating test.
dzearing Feb 13, 2018
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
11 changes: 11 additions & 0 deletions common/changes/@uifabric/utilities/fix-error-handling_2018-02-13-02-38.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "@uifabric/utilities",
"comment": "BaseComponent.onError default implementation removed, exceptions now simply bubble out which lets partners use React 16 error handling.",
"type": "minor"
}
],
"packageName": "@uifabric/utilities",
"email": "[email protected]"
}
76 changes: 0 additions & 76 deletions packages/utilities/src/BaseComponent.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,68 +5,7 @@ import * as React from 'react';
import * as ReactTestUtils from 'react-dom/test-utils';
import { BaseComponent } from './BaseComponent';

let _originalOnError = BaseComponent.onError;

class TestComponent extends BaseComponent<{}, {}> {

public componentWillMount(): void {
this._createNullRef();
}

public componentDidMount(): void {
this._createNullRef();
}

public shouldComponentUpdate(nextProps?: {}, nextState?: {}): boolean {
this._createNullRef();

return true;
}

public componentWillUpdate(): void {
this._createNullRef();
}

public componentWillReceiveProps(): void {
this._createNullRef();
}

public render(): JSX.Element | null {
this._createNullRef();
return null;
}

public componentDidUpdate(): void {
this._createNullRef();
}

public componentWillUnmount(): void {
this._createNullRef();
}

private _createNullRef(): void {
// tslint:disable-next-line:no-any
let foo: any = null;

// Calling a null
foo();
}
}

describe('BaseComponent', () => {
afterEach(() => {
BaseComponent.onError = _originalOnError;
});

_buildTestFor('componentWillMount');
_buildTestFor('componentDidMount');
_buildTestFor('shouldComponentUpdate');
_buildTestFor('componentWillUpdate');
_buildTestFor('componentWillReceiveProps');
_buildTestFor('render');
_buildTestFor('componentDidUpdate');
_buildTestFor('componentWillUnmount');

it('can resolve refs', () => {
class Foo extends BaseComponent<{}, {}> {
public root: HTMLElement;
Expand All @@ -84,18 +23,3 @@ describe('BaseComponent', () => {
expect(component.root).toBeDefined();
});
});

function _buildTestFor(methodName: string): void {
it(`calls the error logger on ${methodName} exception`, () => {
let lastErrorMessage;

BaseComponent.onError = (errorMessage: string | undefined) => lastErrorMessage = errorMessage;

let c = new TestComponent({});

// tslint:disable-next-line:no-any
(c as any)[methodName]();

expect(lastErrorMessage).toBeDefined();
});
}
26 changes: 6 additions & 20 deletions packages/utilities/src/BaseComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ export interface IBaseProps<T = any> {
*/
export class BaseComponent<P extends IBaseProps = {}, S = {}> extends React.Component<P, S> {
/**
* External consumers should override BaseComponent.onError to hook into error messages that occur from
* exceptions thrown from within components.
* @deprecated Use React's error boundaries instead.
*/
// tslint:disable-next-line:no-any
public static onError: ((errorMessage?: string, ex?: any) => void);
Expand Down Expand Up @@ -241,31 +240,18 @@ function _makeSafe(obj: BaseComponent<{}, {}>, prototype: Object, methodName: st
(obj as any)[methodName] = function (): any {
let retVal;

try {
Copy link
Member

@lijunle lijunle Feb 13, 2018

Choose a reason for hiding this comment

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

I suppose this try-catch is the core meaning of this _makeSafe function. Now, after removing this try-catch logic, do we still need this _makeSafe function?

Copy link
Member Author

@dzearing dzearing Feb 13, 2018

Choose a reason for hiding this comment

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

Yes, though it might be more aptly named to something more clear. I was trying to minimize the change as much as possible.

Copy link
Member

@lijunle lijunle Feb 13, 2018

Choose a reason for hiding this comment

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

That sounds good to me!

if (prototypeMethod) {
retVal = prototypeMethod.apply(this, arguments);
}
if (classMethod !== prototypeMethod) {
retVal = classMethod.apply(this, arguments);
}
} catch (e) {
const errorMessage = `Exception in ${obj.className}.${methodName}(): ${typeof e === 'string' ? e : e.stack}`;

if (BaseComponent.onError) {
BaseComponent.onError(errorMessage, e);
}
if (prototypeMethod) {
retVal = prototypeMethod.apply(this, arguments);
}
if (classMethod !== prototypeMethod) {
retVal = classMethod.apply(this, arguments);
}

return retVal;
};
}
}

BaseComponent.onError = (errorMessage: string) => {
console.error(errorMessage);
throw errorMessage;
};

/**
* Simple constant function for returning null, used to render empty templates in JSX.
*
Expand Down