From e295f4a39466eab6353fff08b7b7558f7fa54396 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Wed, 11 Dec 2019 16:11:44 -0800 Subject: [PATCH] Add WarningFilter handling for YellowBox Summary: This diff backports warning filter handling to yellow box. I also backported the skipped warning handling so that ignored patterns do not log to the console, same as LogBox. Changelog: [Internal] Reviewed By: gaearon Differential Revision: D18573288 fbshipit-source-id: 5bf8e86f754adc808313d7ed02f98daaf65de98c --- Libraries/YellowBox/Data/YellowBoxRegistry.js | 54 +++--- .../Data/__tests__/YellowBoxRegistry-test.js | 78 ++++---- Libraries/YellowBox/YellowBox.js | 57 +++++- .../YellowBox/__tests__/YellowBox-test.js | 170 ++++++++++++++++-- 4 files changed, 269 insertions(+), 90 deletions(-) diff --git a/Libraries/YellowBox/Data/YellowBoxRegistry.js b/Libraries/YellowBox/Data/YellowBoxRegistry.js index 95763ab0c7..d430d3fbed 100644 --- a/Libraries/YellowBox/Data/YellowBoxRegistry.js +++ b/Libraries/YellowBox/Data/YellowBoxRegistry.js @@ -12,8 +12,8 @@ const YellowBoxWarning = require('./YellowBoxWarning'); -import type {Category} from './YellowBoxCategory'; - +import type {Category, Message} from './YellowBoxCategory'; +import type {Stack} from './YellowBoxSymbolication'; export type Registry = Map>; export type Observer = (registry: Registry) => void; @@ -32,25 +32,13 @@ let disabled = false; let projection = new Map(); let updateTimeout = null; -function isWarningIgnored(warning: YellowBoxWarning): boolean { - for (const pattern of ignorePatterns) { - if (pattern instanceof RegExp && pattern.test(warning.message.content)) { - return true; - } else if ( - typeof pattern === 'string' && - warning.message.content.includes(pattern) - ) { - return true; - } - } - return false; -} - function handleUpdate(): void { projection = new Map(); if (!disabled) { for (const [category, warnings] of registry) { - const filtered = warnings.filter(warning => !isWarningIgnored(warning)); + const filtered = warnings.filter( + warning => !YellowBoxRegistry.isWarningIgnored(warning.message), + ); if (filtered.length > 0) { projection.set(category, filtered); } @@ -67,18 +55,28 @@ function handleUpdate(): void { } const YellowBoxRegistry = { - add({ - args, - }: $ReadOnly<{| - args: $ReadOnlyArray, - |}>): void { - if (typeof args[0] === 'string' && args[0].startsWith('(ADVICE)')) { - return; + isWarningIgnored(message: Message): boolean { + for (const pattern of ignorePatterns) { + if (pattern instanceof RegExp && pattern.test(message.content)) { + return true; + } else if ( + typeof pattern === 'string' && + message.content.includes(pattern) + ) { + return true; + } } - const {category, message, stack} = YellowBoxWarning.parse({ - args, - }); - + return false; + }, + add({ + category, + message, + stack, + }: $ReadOnly<{| + category: Category, + message: Message, + stack: Stack, + |}>): void { let warnings = registry.get(category); if (warnings == null) { warnings = []; diff --git a/Libraries/YellowBox/Data/__tests__/YellowBoxRegistry-test.js b/Libraries/YellowBox/Data/__tests__/YellowBoxRegistry-test.js index 55be40c1f6..a93f96befd 100644 --- a/Libraries/YellowBox/Data/__tests__/YellowBoxRegistry-test.js +++ b/Libraries/YellowBox/Data/__tests__/YellowBoxRegistry-test.js @@ -11,6 +11,7 @@ 'use strict'; +const YellowBoxWarning = require('../YellowBoxWarning'); const YellowBoxCategory = require('../YellowBoxCategory'); const YellowBoxRegistry = require('../YellowBoxRegistry'); @@ -34,7 +35,7 @@ describe('YellowBoxRegistry', () => { }); it('adds and deletes warnings', () => { - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); const {category: categoryA} = YellowBoxCategory.parse(['A']); expect(registry().size).toBe(1); @@ -46,9 +47,9 @@ describe('YellowBoxRegistry', () => { }); it('clears all warnings', () => { - YellowBoxRegistry.add({args: ['A']}); - YellowBoxRegistry.add({args: ['B']}); - YellowBoxRegistry.add({args: ['C']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['C']})); expect(registry().size).toBe(3); @@ -57,9 +58,9 @@ describe('YellowBoxRegistry', () => { }); it('sorts warnings in chronological order', () => { - YellowBoxRegistry.add({args: ['A']}); - YellowBoxRegistry.add({args: ['B']}); - YellowBoxRegistry.add({args: ['C']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['C']})); const {category: categoryA} = YellowBoxCategory.parse(['A']); const {category: categoryB} = YellowBoxCategory.parse(['B']); @@ -71,7 +72,7 @@ describe('YellowBoxRegistry', () => { categoryC, ]); - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); // Expect `A` to be hoisted to the end of the registry. expect(Array.from(registry().keys())).toEqual([ @@ -82,9 +83,9 @@ describe('YellowBoxRegistry', () => { }); it('ignores warnings matching patterns', () => { - YellowBoxRegistry.add({args: ['A!']}); - YellowBoxRegistry.add({args: ['B?']}); - YellowBoxRegistry.add({args: ['C!']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A!']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B?']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['C!']})); expect(registry().size).toBe(3); YellowBoxRegistry.addIgnorePatterns(['!']); @@ -95,9 +96,9 @@ describe('YellowBoxRegistry', () => { }); it('ignores warnings matching regexs or pattern', () => { - YellowBoxRegistry.add({args: ['There are 4 dogs']}); - YellowBoxRegistry.add({args: ['There are 3 cats']}); - YellowBoxRegistry.add({args: ['There are H cats']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['There are 4 dogs']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['There are 3 cats']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['There are H cats']})); expect(registry().size).toBe(3); YellowBoxRegistry.addIgnorePatterns(['dogs']); @@ -111,9 +112,9 @@ describe('YellowBoxRegistry', () => { }); it('ignores all warnings when disabled', () => { - YellowBoxRegistry.add({args: ['A!']}); - YellowBoxRegistry.add({args: ['B?']}); - YellowBoxRegistry.add({args: ['C!']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A!']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B?']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['C!']})); expect(registry().size).toBe(3); YellowBoxRegistry.setDisabled(true); @@ -124,57 +125,54 @@ describe('YellowBoxRegistry', () => { }); it('groups warnings by simple categories', () => { - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); expect(registry().size).toBe(1); - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); expect(registry().size).toBe(1); - YellowBoxRegistry.add({args: ['B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B']})); expect(registry().size).toBe(2); }); it('groups warnings by format string categories', () => { - YellowBoxRegistry.add({args: ['%s', 'A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['%s', 'A']})); expect(registry().size).toBe(1); - YellowBoxRegistry.add({args: ['%s', 'B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['%s', 'B']})); expect(registry().size).toBe(1); - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); expect(registry().size).toBe(2); - YellowBoxRegistry.add({args: ['B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B']})); expect(registry().size).toBe(3); }); it('groups warnings with consideration for arguments', () => { - YellowBoxRegistry.add({args: ['A', 'B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A', 'B']})); expect(registry().size).toBe(1); - YellowBoxRegistry.add({args: ['A', 'B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A', 'B']})); expect(registry().size).toBe(1); - YellowBoxRegistry.add({args: ['A', 'C']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A', 'C']})); expect(registry().size).toBe(2); - YellowBoxRegistry.add({args: ['%s', 'A', 'A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['%s', 'A', 'A']})); expect(registry().size).toBe(3); - YellowBoxRegistry.add({args: ['%s', 'B', 'A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['%s', 'B', 'A']})); expect(registry().size).toBe(3); - YellowBoxRegistry.add({args: ['%s', 'B', 'B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['%s', 'B', 'B']})); expect(registry().size).toBe(4); }); - it('ignores warnings starting with "(ADVICE)"', () => { - YellowBoxRegistry.add({args: ['(ADVICE) ...']}); - expect(registry().size).toBe(0); - }); - it('does not ignore warnings formatted to start with "(ADVICE)"', () => { - YellowBoxRegistry.add({args: ['%s ...', '(ADVICE)']}); + YellowBoxRegistry.add( + YellowBoxWarning.parse({args: ['%s ...', '(ADVICE)']}), + ); expect(registry().size).toBe(1); }); @@ -189,8 +187,8 @@ describe('YellowBoxRegistry', () => { const {observer} = observe(); expect(observer.mock.calls.length).toBe(1); - YellowBoxRegistry.add({args: ['A']}); - YellowBoxRegistry.add({args: ['B']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['B']})); jest.runAllImmediates(); expect(observer.mock.calls.length).toBe(2); }); @@ -207,7 +205,7 @@ describe('YellowBoxRegistry', () => { const {observer} = observe(); expect(observer.mock.calls.length).toBe(1); - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); jest.runAllImmediates(); expect(observer.mock.calls.length).toBe(2); @@ -226,7 +224,7 @@ describe('YellowBoxRegistry', () => { const {observer} = observe(); expect(observer.mock.calls.length).toBe(1); - YellowBoxRegistry.add({args: ['A']}); + YellowBoxRegistry.add(YellowBoxWarning.parse({args: ['A']})); jest.runAllImmediates(); expect(observer.mock.calls.length).toBe(2); diff --git a/Libraries/YellowBox/YellowBox.js b/Libraries/YellowBox/YellowBox.js index 4e7587583f..fdf0ce6780 100644 --- a/Libraries/YellowBox/YellowBox.js +++ b/Libraries/YellowBox/YellowBox.js @@ -13,6 +13,8 @@ const React = require('react'); import type {Registry, IgnorePattern} from './Data/YellowBoxRegistry'; +import YellowBoxWarning from './Data/YellowBoxWarning'; + import * as LogBoxData from '../LogBox/Data/LogBoxData'; import NativeLogBox from '../NativeModules/specs/NativeLogBox'; @@ -78,15 +80,10 @@ if (__DEV__) { return; } errorImpl = function(...args) { - error.call(console, ...args); - // Show YellowBox for the `warning` module. - if (typeof args[0] === 'string' && args[0].startsWith('Warning: ')) { - registerWarning(...args); - } + registerError(...args); }; warnImpl = function(...args) { - warn.call(console, ...args); registerWarning(...args); }; @@ -151,7 +148,53 @@ if (__DEV__) { }; const registerWarning = (...args): void => { - YellowBoxRegistry.add({args}); + if (typeof args[0] === 'string' && args[0].startsWith('(ADVICE)')) { + return; + } + + const {category, message, stack} = YellowBoxWarning.parse({ + args, + }); + + if (!YellowBoxRegistry.isWarningIgnored(message)) { + YellowBoxRegistry.add({category, message, stack}); + warn.call(console, ...args); + } + }; + + const registerError = (...args): void => { + // Only show YellowBox for the `warning` module, otherwise pass through and skip. + if (typeof args[0] !== 'string' || !args[0].startsWith('Warning: ')) { + error.call(console, ...args); + return; + } + + const format = args[0].replace('Warning: ', ''); + const filterResult = LogBoxData.checkWarningFilter(format); + if (filterResult.suppressCompletely) { + return; + } + + args[0] = filterResult.finalFormat; + const {category, message, stack} = YellowBoxWarning.parse({ + args, + }); + + if (YellowBoxRegistry.isWarningIgnored(message)) { + return; + } + + if (filterResult.forceDialogImmediately === true) { + // This will pop a redbox. Do not downgrade. These are real bugs with same severity as throws. + error.call(console, message.content); + } else { + // Unfortunately, we need to add the Warning: prefix back so we don't show a redbox later. + args[0] = `Warning: ${filterResult.finalFormat}`; + + // Note: YellowBox has no concept of "soft errors" so we're showing YellowBox for those. + YellowBoxRegistry.add({category, message, stack}); + error.call(console, ...args); + } }; } else { YellowBox = class extends React.Component { diff --git a/Libraries/YellowBox/__tests__/YellowBox-test.js b/Libraries/YellowBox/__tests__/YellowBox-test.js index 85eabf1d86..e9a89f3fcd 100644 --- a/Libraries/YellowBox/__tests__/YellowBox-test.js +++ b/Libraries/YellowBox/__tests__/YellowBox-test.js @@ -12,7 +12,6 @@ 'use strict'; import * as React from 'react'; -const YellowBox = require('../YellowBox'); const YellowBoxRegistry = require('../Data/YellowBoxRegistry'); const LogBoxData = require('../../LogBox/Data/LogBoxData'); const render = require('../../../jest/renderer'); @@ -22,17 +21,52 @@ jest.mock('../../LogBox/LogBoxNotificationContainer', () => ({ default: 'LogBoxNotificationContainer', })); +type Overrides = {| + forceDialogImmediately?: boolean, + suppressDialog_LEGACY?: boolean, + suppressCompletely?: boolean, +|}; + +const setFilter = (options?: Overrides) => { + LogBoxData.setWarningFilter(format => ({ + finalFormat: format, + forceDialogImmediately: false, + suppressDialog_LEGACY: false, + suppressCompletely: false, + monitorEvent: null, + monitorListVersion: 0, + monitorSampleRate: 0, + ...options, + })); +}; + +const install = () => { + const YellowBox = require('../YellowBox'); + YellowBox.install(); +}; + +const uninstall = () => { + const YellowBox = require('../YellowBox'); + YellowBox.uninstall(); +}; + describe('YellowBox', () => { const {error, warn} = console; + const mockError = jest.fn(); + const mockWarn = jest.fn(); beforeEach(() => { jest.resetModules(); - (console: any).error = jest.fn(); - (console: any).warn = jest.fn(); + + mockError.mockClear(); + mockWarn.mockClear(); + + (console: any).error = mockError; + (console: any).warn = mockWarn; }); afterEach(() => { - YellowBox.uninstall(); + uninstall(); (console: any).error = error; (console: any).warn = warn; }); @@ -40,7 +74,7 @@ describe('YellowBox', () => { it('can set `disableYellowBox` after installing', () => { expect((console: any).disableYellowBox).toBe(undefined); - YellowBox.install(); + install(); expect((console: any).disableYellowBox).toBe(false); expect(YellowBoxRegistry.isDisabled()).toBe(false); @@ -55,7 +89,7 @@ describe('YellowBox', () => { expect((console: any).disableYellowBox).toBe(undefined); (console: any).disableYellowBox = true; - YellowBox.install(); + install(); expect((console: any).disableYellowBox).toBe(true); expect(YellowBoxRegistry.isDisabled()).toBe(true); @@ -64,38 +98,140 @@ describe('YellowBox', () => { it('registers warnings', () => { jest.mock('../Data/YellowBoxRegistry'); - YellowBox.install(); + install(); expect(YellowBoxRegistry.add).not.toBeCalled(); (console: any).warn('...'); expect(YellowBoxRegistry.add).toBeCalled(); + expect(mockWarn).toBeCalledTimes(1); + expect(mockWarn).toBeCalledWith('...'); }); - it('registers errors beginning with "Warning: "', () => { + it('registers errors', () => { jest.mock('../Data/YellowBoxRegistry'); - YellowBox.install(); + install(); (console: any).error('...'); expect(YellowBoxRegistry.add).not.toBeCalled(); + expect(mockError).toBeCalledTimes(1); + expect(mockError).toBeCalledWith('...'); + }); + + it('skips ADVICE warnings', () => { + jest.mock('../Data/YellowBoxRegistry'); + + install(); + + (console: any).warn('(ADVICE) Ignore me'); + expect(YellowBoxRegistry.add).not.toBeCalled(); + expect(mockWarn).not.toBeCalled(); + }); + + it('skips ignored warnings', () => { + jest.mock('../Data/YellowBoxRegistry'); + + install(); + + (YellowBoxRegistry: any).isWarningIgnored.mockReturnValue(true); + (console: any).warn('Ignore me'); + expect(YellowBoxRegistry.add).not.toBeCalled(); + expect(mockWarn).not.toBeCalled(); + }); + + it('registers Warning module errors with default options to YellowBox', () => { + jest.mock('../Data/YellowBoxRegistry'); + + setFilter(); + install(); (console: any).error('Warning: ...'); expect(YellowBoxRegistry.add).toBeCalled(); + expect(mockError).toBeCalled(); + expect(mockError).toBeCalledTimes(1); + expect(mockWarn).not.toBeCalled(); + }); + + it('skips Warning module errors with forceDialogImmediately', () => { + jest.mock('../Data/YellowBoxRegistry'); + + setFilter({ + suppressCompletely: true, + }); + install(); + + (console: any).error('Warning: ...'); + expect(YellowBoxRegistry.add).not.toBeCalled(); + expect(mockError).not.toBeCalled(); + expect(mockWarn).not.toBeCalled(); + }); + + it('registers Warning errors with forceDialogImmediately as console.error (with interpolation)', () => { + jest.mock('../Data/YellowBoxRegistry'); + + setFilter({ + forceDialogImmediately: true, + }); + + install(); + + (console: any).error('Warning: %s', 'Something'); + expect(YellowBoxRegistry.add).not.toBeCalled(); + expect(mockWarn).not.toBeCalled(); + expect(mockError).toBeCalledTimes(1); + + // We expect this to be the interpolated value because we don't do interpolation downstream. + // We also strip the "Warning" prefix, otherwise the redbox would be skipped downstream. + expect(mockError).toBeCalledWith('Something'); + }); + + it('registers Warning errors with suppressDialog_LEGACY to YellowBox', () => { + jest.mock('../Data/YellowBoxRegistry'); + + setFilter({ + suppressDialog_LEGACY: true, + }); + + install(); + + (console: any).error('Warning: Something'); + expect(YellowBoxRegistry.add).toBeCalledTimes(1); + expect(mockWarn).not.toBeCalled(); + expect(mockError).toBeCalledTimes(1); + + // We cannot strip the "Warning" prefix or it would pop a redbox. + expect(mockError).toBeCalledWith('Warning: Something'); + }); + + it('skips Warning errors sent to YellowBox but ignored by patterns', () => { + jest.mock('../Data/YellowBoxRegistry'); + + setFilter({ + suppressDialog_LEGACY: true, + }); + + install(); + (YellowBoxRegistry: any).isWarningIgnored.mockReturnValue(true); + + (console: any).error('Warning: ...'); + expect(YellowBoxRegistry.add).not.toBeCalled(); + expect(mockWarn).not.toBeCalled(); + expect(mockError).not.toBeCalled(); }); it('if LogBox is enabled, installs and uninstalls LogBox', () => { jest.mock('../../LogBox/Data/LogBoxData'); jest.mock('../Data/YellowBoxRegistry'); - + const YellowBox = require('../YellowBox'); YellowBox.__unstable_enableLogBox(); - YellowBox.install(); + install(); (console: any).warn('Some warning'); expect(YellowBoxRegistry.add).not.toBeCalled(); expect(LogBoxData.addLog).toBeCalled(); - expect(YellowBox.__unstable_isLogBoxEnabled()).toBe(true); + expect(require('../YellowBox').__unstable_isLogBoxEnabled()).toBe(true); - YellowBox.uninstall(); + uninstall(); (LogBoxData.addLog: any).mockClear(); (console: any).warn('Some warning'); @@ -106,8 +242,8 @@ describe('YellowBox', () => { it('throws if LogBox is enabled after YellowBox is installed', () => { jest.mock('../Data/YellowBoxRegistry'); - - YellowBox.install(); + const YellowBox = require('../YellowBox'); + install(); expect(() => YellowBox.__unstable_enableLogBox()).toThrow( 'LogBox must be enabled before AppContainer is required so that it can properly wrap the console methods.\n\nPlease enable LogBox earlier in your app.\n\n', @@ -115,12 +251,16 @@ describe('YellowBox', () => { }); it('should render YellowBoxContainer by default', () => { + const YellowBox = require('../YellowBox'); + const output = render.shallowRender(); expect(output).toMatchSnapshot(); }); it('should render LogBoxNotificationContainer when LogBox is enabled', () => { + const YellowBox = require('../YellowBox'); + YellowBox.__unstable_enableLogBox(); const output = render.shallowRender();