-
Notifications
You must be signed in to change notification settings - Fork 25.1k
show code line in warning #7030
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| */ | ||
| 'use strict'; | ||
|
|
||
| const SourceMapsCache = jest.genMockFromModule('SourceMapsCache'); | ||
|
|
||
| var getSourceMaps = jest.genMockFunction(); | ||
| const promise = new Promise((resolve, reject) => resolve({})); | ||
|
|
||
| getSourceMaps.mockReturnValue(promise); | ||
| SourceMapsCache.getSourceMaps = getSourceMaps; | ||
|
|
||
| module.exports = SourceMapsCache; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| */ | ||
| 'use strict'; | ||
|
|
||
| const parseErrorStack = jest.genMockFunction(); | ||
|
|
||
| const frame = [{ | ||
| file: '/Examples/UIExplorer/ViewExample.js', | ||
| methodName: 'Constructor.render', | ||
| lineNumber: 42, | ||
| column: 0 | ||
| }]; | ||
|
|
||
| parseErrorStack.mockReturnValue(frame); | ||
|
|
||
| module.exports = parseErrorStack; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| */ | ||
| 'use strict'; | ||
|
|
||
| jest.unmock('../exceptionOccurHint'); | ||
|
|
||
| const exceptionOccurHint = require('exceptionOccurHint'); | ||
|
|
||
| describe('exceptionOccurHint', () => { | ||
|
|
||
| pit('give a hint', () => { | ||
| return exceptionOccurHint().then(hint => { | ||
| expect(hint).toEqual('At: ViewExample.js:42'); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| * | ||
| * @providesModule exceptionOccurHint | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| const SourceMapsCache = require('SourceMapsCache'); | ||
| const parseErrorStack = require('parseErrorStack'); | ||
|
|
||
| function exceptionOccurHint() { | ||
| const stack = parseErrorStack(new Error()); | ||
| const renderFrame = stack.find((frame) => frame.methodName.endsWith('render')); | ||
|
|
||
| if (renderFrame) { | ||
| return SourceMapsCache.getSourceMaps().then(sourceMaps => { | ||
| const prettyStack = parseErrorStack({stack: [renderFrame]}, sourceMaps); | ||
| const prettyFrame = prettyStack[0]; | ||
| const fileParts = prettyFrame.file.split('/'); | ||
| const fileName = fileParts[fileParts.length - 1]; | ||
| return `At: ${fileName }:${prettyFrame.lineNumber}`; | ||
| }); | ||
| } else { | ||
| return new Promise((resolve, reject) => resolve(null)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if renderFrame doesn't exist, let's just return the file/line where the console.warn is, and also include the pretty stack after the whole error message. |
||
| } | ||
| } | ||
|
|
||
| module.exports = exceptionOccurHint; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import type EmitterSubscription from 'EmitterSubscription'; | |
| const Platform = require('Platform'); | ||
| const React = require('React'); | ||
| const StyleSheet = require('StyleSheet'); | ||
| const exceptionOccurHint = require('exceptionOccurHint'); | ||
|
|
||
| const _warningEmitter = new EventEmitter(); | ||
| const _warningMap = new Map(); | ||
|
|
@@ -82,9 +83,23 @@ function updateWarningMap(format, ...args): void { | |
| ...args.slice(argCount).map(stringifySafe), | ||
| ].join(' '); | ||
|
|
||
| const count = _warningMap.has(warning) ? _warningMap.get(warning) : 0; | ||
| _warningMap.set(warning, count + 1); | ||
| var warningInfo = _warningMap.get(warning); | ||
| if (warningInfo) { | ||
| warningInfo.count += 1; | ||
| } else { | ||
| warningInfo = {count: 1, frame: ''}; | ||
| } | ||
|
|
||
| _warningMap.set(warning, warningInfo); | ||
| _warningEmitter.emit('warning', _warningMap); | ||
|
|
||
| exceptionOccurHint().then((frame) => { | ||
| warningInfo = _warningMap.get(warning); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property |
||
| if (warningInfo && frame) { | ||
| warningInfo.frame = frame; | ||
| _warningEmitter.emit('warning', _warningMap); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function isWarningIgnored(warning: string): boolean { | ||
|
|
@@ -123,6 +138,7 @@ const WarningRow = ({count, warning, onPress}) => { | |
|
|
||
| const WarningInspector = ({ | ||
| count, | ||
| frame, | ||
| warning, | ||
| onClose, | ||
| onDismiss, | ||
|
|
@@ -145,6 +161,7 @@ const WarningInspector = ({ | |
| <View style={styles.inspectorContent}> | ||
| <View style={styles.inspectorCount}> | ||
| <Text style={styles.inspectorCountText}>{countSentence}</Text> | ||
| <Text style={styles.inspectorCountText}>{frame}</Text> | ||
| </View> | ||
| <ScrollView style={styles.inspectorWarning}> | ||
| <Text style={styles.inspectorWarningText}>{warning}</Text> | ||
|
|
@@ -230,9 +247,12 @@ class YellowBox extends React.Component { | |
| const View = require('View'); | ||
|
|
||
| const inspecting = this.state.inspecting; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property |
||
| const inspector = inspecting !== null ? | ||
| const inspectingWarningInfo = this.state.warningMap.get(inspecting); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property |
||
| const inspector = inspecting !== null && inspectingWarningInfo !== null ? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. property |
||
| <WarningInspector | ||
| count={this.state.warningMap.get(inspecting)} | ||
| count={inspectingWarningInfo.count} | ||
| frame={inspectingWarningInfo.frame} | ||
| warning={inspecting} | ||
| onClose={() => this.setState({inspecting: null})} | ||
| onDismiss={() => this.dismissWarning(inspecting)} | ||
|
|
@@ -241,12 +261,12 @@ class YellowBox extends React.Component { | |
| null; | ||
|
|
||
| const rows = []; | ||
| this.state.warningMap.forEach((count, warning) => { | ||
| this.state.warningMap.forEach((warningInfo, warning) => { | ||
| if (!isWarningIgnored(warning)) { | ||
| rows.push( | ||
| <WarningRow | ||
| key={warning} | ||
| count={count} | ||
| count={warningInfo.count} | ||
| warning={warning} | ||
| onPress={() => this.setState({inspecting: warning})} | ||
| onDismiss={() => this.dismissWarning(warning)} | ||
|
|
@@ -324,7 +344,7 @@ var styles = StyleSheet.create({ | |
| inspectorWarning: { | ||
| padding: 15, | ||
| position: 'absolute', | ||
| top: 39, | ||
| top: 50, | ||
| bottom: 60, | ||
| }, | ||
| inspectorWarningText: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why search for render rather than skip a fixed number of frames?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain how can I know that render method happens at fixed number of frames? For me search for method
rendermakes more sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I tried to show warnings for properties checking. For that case
rendermethod is a right hint. Now I'm looking for other warnings. Not sure how to deal with it yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I see - you don't want to report the line where the warning was fired since that's in the infra code...maybe we could just give the line of the warning by default, but have some custom logic for the props case you're focused on here - does that make sense? Basically if the stack looks like it comes from the strict prop type checker then we grab the render function, otherwise we just show the file/line of the actual warning.
Also, we could just include the entire parsed error stack at the bottom of the warning message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this whole thing is weird because the strict prop checker invariant should throw an exception anyway, not a warning, not sure what's up with that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an awesome idea. We should show user line of code. For
react-nativeapps it is easy - everything not insidenode_modules. But for examples https://github.com/facebook/react-native/tree/master/Examples it is not so easy becausereact-nativenot undernode_modulesfolder. Is there any way to get app root directory?