Remove framesToPop from YellowBox

Summary: Removes the use of `framesToPop` to manage frame skipping in YellowBox and replaces it with support for the `collapse` field populated by Metro's [`customizeFrame`](facebook/metro#435) config option. `framesToPop` is a deprecated mechanism which will be removed in the future. Reviewed By: bvaughn Differential Revision: D17857057 fbshipit-source-id: 120383ba4aad877b7ca79c7cf9d5b7579a490577
facebook · Oct 10, 2019 · cf4d45e · cf4d45e
1 parent df96de7
commit cf4d45e
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 59 deletions.
diff --git a/Libraries/YellowBox/Data/YellowBoxRegistry.js b/Libraries/YellowBox/Data/YellowBoxRegistry.js
@@ -69,17 +69,14 @@ function handleUpdate(): void {
 const YellowBoxRegistry = {
   add({
     args,
-    framesToPop,
   }: $ReadOnly<{|
     args: $ReadOnlyArray<mixed>,
-    framesToPop: number,
   |}>): void {
     if (typeof args[0] === 'string' && args[0].startsWith('(ADVICE)')) {
       return;
     }
     const {category, message, stack} = YellowBoxWarning.parse({
       args,
-      framesToPop: framesToPop + 1,
     });
 
     let warnings = registry.get(category);

diff --git a/Libraries/YellowBox/Data/YellowBoxSymbolication.js b/Libraries/YellowBox/Data/YellowBoxSymbolication.js
@@ -66,11 +66,19 @@ const sanitize = (maybeStack: mixed): Stack => {
     if (typeof maybeFrame.methodName !== 'string') {
       throw new Error('Expected stack frame `methodName` to be a string.');
     }
+    let collapse = false;
+    if ('collapse' in maybeFrame) {
+      if (typeof maybeFrame.collapse !== 'boolean') {
+        throw new Error('Expected stack frame `collapse` to be a boolean.');
+      }
+      collapse = maybeFrame.collapse;
+    }
     stack.push({
       column: maybeFrame.column,
       file: maybeFrame.file,
       lineNumber: maybeFrame.lineNumber,
       methodName: maybeFrame.methodName,
+      collapse,
     });
   }
   return stack;

diff --git a/Libraries/YellowBox/Data/YellowBoxWarning.js b/Libraries/YellowBox/Data/YellowBoxWarning.js
@@ -25,10 +25,8 @@ export type SymbolicationRequest = $ReadOnly<{|
 class YellowBoxWarning {
   static parse({
     args,
-    framesToPop,
   }: $ReadOnly<{|
     args: $ReadOnlyArray<mixed>,
-    framesToPop: number,
   |}>): {|
     category: Category,
     message: Message,
@@ -56,7 +54,8 @@ class YellowBoxWarning {
 
     return {
       ...YellowBoxCategory.parse(mutableArgs),
-      stack: createStack({framesToPop: framesToPop + 1}),
+      // TODO: Use Error.captureStackTrace on Hermes
+      stack: parseErrorStack(new Error()),
     };
   }
 
@@ -124,10 +123,4 @@ class YellowBoxWarning {
   }
 }
 
-function createStack({framesToPop}: $ReadOnly<{|framesToPop: number|}>): Stack {
-  const error: any = new Error();
-  error.framesToPop = framesToPop + 1;
-  return parseErrorStack(error);
-}
-
 module.exports = YellowBoxWarning;
diff --git a/Libraries/YellowBox/Data/__tests__/YellowBoxRegistry-test.js b/Libraries/YellowBox/Data/__tests__/YellowBoxRegistry-test.js
@@ -34,7 +34,7 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('adds and deletes warnings', () => {
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
     const {category: categoryA} = YellowBoxCategory.parse(['A']);
 
     expect(registry().size).toBe(1);
@@ -46,9 +46,9 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('clears all warnings', () => {
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['B'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['C'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
+    YellowBoxRegistry.add({args: ['B']});
+    YellowBoxRegistry.add({args: ['C']});
 
     expect(registry().size).toBe(3);
 
@@ -57,9 +57,9 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('sorts warnings in chronological order', () => {
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['B'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['C'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
+    YellowBoxRegistry.add({args: ['B']});
+    YellowBoxRegistry.add({args: ['C']});
 
     const {category: categoryA} = YellowBoxCategory.parse(['A']);
     const {category: categoryB} = YellowBoxCategory.parse(['B']);
@@ -71,7 +71,7 @@ describe('YellowBoxRegistry', () => {
       categoryC,
     ]);
 
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
 
     // Expect `A` to be hoisted to the end of the registry.
     expect(Array.from(registry().keys())).toEqual([
@@ -82,9 +82,9 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('ignores warnings matching patterns', () => {
-    YellowBoxRegistry.add({args: ['A!'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['B?'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['C!'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A!']});
+    YellowBoxRegistry.add({args: ['B?']});
+    YellowBoxRegistry.add({args: ['C!']});
     expect(registry().size).toBe(3);
 
     YellowBoxRegistry.addIgnorePatterns(['!']);
@@ -95,9 +95,9 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('ignores warnings matching regexs or pattern', () => {
-    YellowBoxRegistry.add({args: ['There are 4 dogs'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['There are 3 cats'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['There are H cats'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['There are 4 dogs']});
+    YellowBoxRegistry.add({args: ['There are 3 cats']});
+    YellowBoxRegistry.add({args: ['There are H cats']});
     expect(registry().size).toBe(3);
 
     YellowBoxRegistry.addIgnorePatterns(['dogs']);
@@ -111,9 +111,9 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('ignores all warnings when disabled', () => {
-    YellowBoxRegistry.add({args: ['A!'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['B?'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['C!'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A!']});
+    YellowBoxRegistry.add({args: ['B?']});
+    YellowBoxRegistry.add({args: ['C!']});
     expect(registry().size).toBe(3);
 
     YellowBoxRegistry.setDisabled(true);
@@ -124,57 +124,57 @@ describe('YellowBoxRegistry', () => {
   });
 
   it('groups warnings by simple categories', () => {
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
     expect(registry().size).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
     expect(registry().size).toBe(1);
 
-    YellowBoxRegistry.add({args: ['B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['B']});
     expect(registry().size).toBe(2);
   });
 
   it('groups warnings by format string categories', () => {
-    YellowBoxRegistry.add({args: ['%s', 'A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['%s', 'A']});
     expect(registry().size).toBe(1);
 
-    YellowBoxRegistry.add({args: ['%s', 'B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['%s', 'B']});
     expect(registry().size).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
     expect(registry().size).toBe(2);
 
-    YellowBoxRegistry.add({args: ['B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['B']});
     expect(registry().size).toBe(3);
   });
 
   it('groups warnings with consideration for arguments', () => {
-    YellowBoxRegistry.add({args: ['A', 'B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A', 'B']});
     expect(registry().size).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A', 'B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A', 'B']});
     expect(registry().size).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A', 'C'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A', 'C']});
     expect(registry().size).toBe(2);
 
-    YellowBoxRegistry.add({args: ['%s', 'A', 'A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['%s', 'A', 'A']});
     expect(registry().size).toBe(3);
 
-    YellowBoxRegistry.add({args: ['%s', 'B', 'A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['%s', 'B', 'A']});
     expect(registry().size).toBe(3);
 
-    YellowBoxRegistry.add({args: ['%s', 'B', 'B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['%s', 'B', 'B']});
     expect(registry().size).toBe(4);
   });
 
   it('ignores warnings starting with "(ADVICE)"', () => {
-    YellowBoxRegistry.add({args: ['(ADVICE) ...'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['(ADVICE) ...']});
     expect(registry().size).toBe(0);
   });
 
   it('does not ignore warnings formatted to start with "(ADVICE)"', () => {
-    YellowBoxRegistry.add({args: ['%s ...', '(ADVICE)'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['%s ...', '(ADVICE)']});
     expect(registry().size).toBe(1);
   });
 
@@ -189,8 +189,8 @@ describe('YellowBoxRegistry', () => {
     const {observer} = observe();
     expect(observer.mock.calls.length).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
-    YellowBoxRegistry.add({args: ['B'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
+    YellowBoxRegistry.add({args: ['B']});
     jest.runAllImmediates();
     expect(observer.mock.calls.length).toBe(2);
   });
@@ -207,7 +207,7 @@ describe('YellowBoxRegistry', () => {
     const {observer} = observe();
     expect(observer.mock.calls.length).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
     jest.runAllImmediates();
     expect(observer.mock.calls.length).toBe(2);
 
@@ -226,7 +226,7 @@ describe('YellowBoxRegistry', () => {
     const {observer} = observe();
     expect(observer.mock.calls.length).toBe(1);
 
-    YellowBoxRegistry.add({args: ['A'], framesToPop: 0});
+    YellowBoxRegistry.add({args: ['A']});
     jest.runAllImmediates();
     expect(observer.mock.calls.length).toBe(2);
 

diff --git a/Libraries/YellowBox/UI/YellowBoxInspector.js b/Libraries/YellowBox/UI/YellowBoxInspector.js
@@ -86,7 +86,10 @@ class YellowBoxInspector extends React.Component<Props, State> {
               />
             </View>
             {warning.getAvailableStack().map((frame, index) => {
-              const {file, lineNumber} = frame;
+              const {file, lineNumber, collapse = false} = frame;
+              if (collapse) {
+                return null;
+              }
               return (
                 <YellowBoxInspectorStackFrame
                   key={index}

diff --git a/Libraries/YellowBox/YellowBox.js b/Libraries/YellowBox/YellowBox.js
@@ -145,16 +145,7 @@ if (__DEV__) {
   };
 
   const registerWarning = (...args): void => {
-    // YellowBox should ignore the top 3-4 stack frames:
-    // 1: registerWarning() itself
-    // 2: YellowBox's own console override (in this file)
-    // 3: React DevTools console.error override (to add component stack)
-    //    (The override feature may be disabled by a runtime preference.)
-    // 4: The actual console method itself.
-    // $FlowFixMe This prop is how the DevTools override is observable.
-    const isDevToolsOvveride = !!console.warn
-      .__REACT_DEVTOOLS_ORIGINAL_METHOD__;
-    YellowBoxRegistry.add({args, framesToPop: isDevToolsOvveride ? 4 : 3});
+    YellowBoxRegistry.add({args});
   };
 } else {
   YellowBox = class extends React.Component<Props, State> {