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.

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

Provide non-standard stack with invalid type warnings #9679

Merged
merged 5 commits into from
May 15, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 2 additions & 0 deletions .flowconfig
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
<PROJECT_ROOT>/examples/.*
<PROJECT_ROOT>/fixtures/.*
<PROJECT_ROOT>/build/.*
<PROJECT_ROOT>/node_modules/chrome-devtools-frontend/.*
<PROJECT_ROOT>/.*/node_modules/chrome-devtools-frontend/.*
<PROJECT_ROOT>/.*/node_modules/y18n/.*
<PROJECT_ROOT>/.*/__mocks__/.*
<PROJECT_ROOT>/.*/__tests__/.*
Expand Down
7 changes: 7 additions & 0 deletions src/isomorphic/classic/element/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,12 @@ var ReactElementValidator = {

info += ReactComponentTreeHook.getCurrentStackAddendum();

var currentSource = props !== null &&
props !== undefined &&
props.__source !== undefined
? props.__source
: null;
ReactComponentTreeHook.pushNonStandardWarningStack(true, currentSource);
warning(
false,
'React.createElement: type is invalid -- expected a string (for ' +
Expand All @@ -231,6 +237,7 @@ var ReactElementValidator = {
type == null ? type : typeof type,
info,
);
ReactComponentTreeHook.popNonStandardWarningStack();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -525,4 +525,55 @@ describe('ReactElementValidator', () => {
"component from the file it's defined in. Check your code at **.",
);
});

it('provides stack via non-standard console.stack for invalid types', () => {
spyOn(console, 'error');

function Foo() {
var Bad = undefined;
return React.createElement(Bad);
}

function App() {
return React.createElement('div', null, React.createElement(Foo));
}

try {
console.stack = jest.fn();
console.stackEnd = jest.fn();

expect(() => {
ReactTestUtils.renderIntoDocument(React.createElement(App));
}).toThrow(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined. ' +
"You likely forgot to export your component from the file it's " +
'defined in. Check the render method of `Foo`.',
);

expect(console.stack.mock.calls.length).toBe(1);
expect(console.stackEnd.mock.calls.length).toBe(1);

var stack = console.stack.mock.calls[0][0];
expect(Array.isArray(stack)).toBe(true);
expect(stack.map(frame => frame.name)).toEqual([
'Foo', // <Bad> is inside Foo
'App', // <Foo> is inside App
'App', // <div> is inside App
null, // <App> is outside a component
]);
expect(
stack.map(frame => frame.fileName && frame.fileName.slice(-8)),
).toEqual([null, null, null, null]);
expect(stack.map(frame => frame.lineNumber)).toEqual([
null,
null,
null,
null,
]);
} finally {
delete console.stack;
delete console.stackEnd;
}
});
});
51 changes: 51 additions & 0 deletions src/isomorphic/hooks/ReactComponentTreeHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,57 @@ var ReactComponentTreeHook = {

getRootIDs,
getRegisteredIDs: getItemIDs,

pushNonStandardWarningStack(
isCreatingElement: boolean,
currentSource: ?Source,
) {
if (typeof console.stack !== 'function') {
return;
}

var stack = [];
var currentOwner = ReactCurrentOwner.current;
var id = currentOwner && currentOwner._debugID;

try {
if (isCreatingElement) {
stack.push({
name: id ? ReactComponentTreeHook.getDisplayName(id) : null,
fileName: currentSource ? currentSource.fileName : null,
lineNumber: currentSource ? currentSource.lineNumber : null,
});
}

while (id) {
var element = ReactComponentTreeHook.getElement(id);
var parentID = ReactComponentTreeHook.getParentID(id);
var ownerID = ReactComponentTreeHook.getOwnerID(id);
var ownerName = ownerID
? ReactComponentTreeHook.getDisplayName(ownerID)
: null;
var source = element && element._source;
stack.push({
name: ownerName,
fileName: source ? source.fileName : null,
lineNumber: source ? source.lineNumber : null,
});
id = parentID;
}
} catch (err) {
// Internal state is messed up.
// Stop building the stack (it's just a nice to have).
}

console.stack(stack);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking that it would be good if console.stack indicated the root stack frame.

That way the host is welcome to add more stack frames on top of it. Essentially the same as storing new Error().stack and stripping off the location of the next new Error() before appending the custom stack.

This is useful for things like custom Promise, Algebraic Effects or call/cc implementations. E.g. Where you can to begin doing the "thenable" work you would call console.stack with the stack that caused the Promise to be created.

This makes sense if the console.stack frame is collocated with console.warning but that might be too limited.

So perhaps the semantics should be that any common stack frame is considered the root until the console.endStack frame? That seems doable.

Effectively that would mean in our case that the host environment can visualize the warning() and console.warn() call frames on top of the custom one.

},

popNonStandardWarningStack() {
if (typeof console.stackEnd !== 'function') {
return;
}
console.stackEnd();
},
};

module.exports = ReactComponentTreeHook;
Original file line number Diff line number Diff line change
Expand Up @@ -400,4 +400,55 @@ describe('ReactJSXElementValidator', () => {
' Use a static property named `defaultProps` instead.',
);
});

it('provides stack via non-standard console.stack for invalid types', () => {
spyOn(console, 'error');

function Foo() {
var Bad = undefined;
return <Bad />;
}

function App() {
return <div><Foo /></div>;
}

try {
console.stack = jest.fn();
console.stackEnd = jest.fn();

expect(() => {
ReactTestUtils.renderIntoDocument(<App />);
}).toThrow(
'Element type is invalid: expected a string (for built-in components) ' +
'or a class/function (for composite components) but got: undefined. ' +
"You likely forgot to export your component from the file it's " +
'defined in. Check the render method of `Foo`.',
);

expect(console.stack.mock.calls.length).toBe(1);
expect(console.stackEnd.mock.calls.length).toBe(1);

var stack = console.stack.mock.calls[0][0];
expect(Array.isArray(stack)).toBe(true);
expect(stack.map(frame => frame.name)).toEqual([
'Foo', // <Bad> is inside Foo
'App', // <Foo> is inside App
'App', // <div> is inside App
null, // <App> is outside a component
]);
expect(
stack.map(frame => frame.fileName && frame.fileName.slice(-8)),
).toEqual(['-test.js', '-test.js', '-test.js', '-test.js']);
expect(stack.map(frame => typeof frame.lineNumber)).toEqual([
'number',
'number',
'number',
'number',
]);
} finally {
delete console.stack;
delete console.stackEnd;
}
});
});