bahman/react
1
0
Fork 0
Code Issues Pull Requests Actions 6 Packages Projects Releases Wiki Activity

Replace wrap-warning-with-env-check with an eslint plugin (#17540)

Browse Source 
* Replace Babel plugin with an ESLint plugin

* Fix ESLint rule violations

* Move shared conditions higher

* Test formatting nits

* Tweak ESLint rule

* Bugfix: inside else branch, 'if' tests are not satisfactory

* Use a stricter check for exactly if (__DEV__)

This makes it easier to see what's going on and matches dominant style in the codebase.

* Fix remaining files after stricter check
This commit is contained in:
Laura buns
2019-12-06 18:25:54 +00:00
committed by Dan Abramov
parent acfe4b21b2
commit b43eec7eaa
35 changed files with 933 additions and 550 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
scripts/babel/__tests__/wrap-warning-with-env-check-test.js → scripts/babel/__tests__/lift-warning-conditional-argument-test.js
View File

@@ -8,7 +8,7 @@
'use strict';
let babel = require('@babel/core');
let wrapWarningWithEnvCheck = require('../wrap-warning-with-env-check');
let wrapWarningWithEnvCheck = require('../lift-warning-conditional-argument');
function transform(input) {
return babel.transform(input, {
@@ -23,7 +23,7 @@ function compare(input, output) {
let oldEnv;
describe('wrap-warning-with-env-check', () => {
describe('lift-warning-conditional-argument', () => {
beforeEach(() => {
oldEnv = process.env.NODE_ENV;
process.env.NODE_ENV = '';
@@ -36,14 +36,14 @@ describe('wrap-warning-with-env-check', () => {
it('should wrap warning calls', () => {
compare(
"warning(condition, 'a %s b', 'c');",
"__DEV__ ? !condition ? warning(false, 'a %s b', 'c') : void 0 : void 0;"
"!condition ? warning(false, 'a %s b', 'c') : void 0;"
);
});
it('should wrap warningWithoutStack calls', () => {
compare(
"warningWithoutStack(condition, 'a %s b', 'c');",
"__DEV__ ? !condition ? warningWithoutStack(false, 'a %s b', 'c') : void 0 : void 0;"
"!condition ? warningWithoutStack(false, 'a %s b', 'c') : void 0;"
);
});

17
scripts/babel/wrap-warning-with-env-check.js → scripts/babel/lift-warning-conditional-argument.js
View File

@@ -9,8 +9,6 @@
module.exports = function(babel, options) {
const t = babel.types;
const DEV_EXPRESSION = t.identifier('__DEV__');
const SEEN_SYMBOL = Symbol('expression.seen');
return {
@@ -34,10 +32,8 @@ module.exports = function(babel, options) {
//
// into this:
//
// if (__DEV__) {
// if (!condition) {
// warning(false, argument, argument);
// }
// if (!condition) {
// warning(false, argument, argument);
// }
//
// The goal is to strip out warning calls entirely in production
@@ -50,13 +46,8 @@ module.exports = function(babel, options) {
newNode[SEEN_SYMBOL] = true;
path.replaceWith(
t.ifStatement(
DEV_EXPRESSION,
t.blockStatement([
t.ifStatement(
t.unaryExpression('!', condition),
t.expressionStatement(newNode)
),
])
t.unaryExpression('!', condition),
t.expressionStatement(newNode)
)
);
}

234
scripts/eslint-rules/__tests__/no-production-logging-test.internal.js Normal file
View File

@@ -0,0 +1,234 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/
'use strict';
const rule = require('../no-production-logging');
const RuleTester = require('eslint').RuleTester;
const ruleTester = new RuleTester();
ruleTester.run('no-production-logging', rule, {
valid: [
{
code: `
if (__DEV__) {
warning(test, 'Oh no');
}
`,
},
{
code: `
if (__DEV__) {
warningWithoutStack(test, 'Oh no');
}
`,
},
{
code: `
if (__DEV__) {
lowPriorityWarning(test, 'Oh no');
}
`,
},
{
code: `
if (__DEV__) {
lowPriorityWarningWithoutStack(test, 'Oh no');
}
`,
},
// This is OK too because it's wrapped outside:
{
code: `
if (__DEV__) {
if (potato) {
while (true) {
warning(test, 'Oh no');
}
}
}`,
},
{
code: `
var f;
if (__DEV__) {
f = function() {
if (potato) {
while (true) {
warning(test, 'Oh no');
}
}
};
}`,
},
// Don't do anything with these:
{
code: 'normalFunctionCall(test);',
},
{
code: 'invariant(test);',
},
{
code: `
if (__DEV__) {
normalFunctionCall(test);
}
`,
},
// This is OK because of the outer if.
{
code: `
if (__DEV__) {
if (foo) {
if (__DEV__) {
} else {
warning(test, 'Oh no');
}
}
}`,
},
],
invalid: [
{
code: 'warning(test);',
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: 'warningWithoutStack(test)',
errors: [
{
message: `Wrap warningWithoutStack() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (potato) {
warningWithoutStack(test);
}
`,
errors: [
{
message: `Wrap warningWithoutStack() in an "if (__DEV__) {}" check`,
},
],
},
{
code: 'lowPriorityWarning(test);',
errors: [
{
message: `Wrap lowPriorityWarning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: 'lowPriorityWarningWithoutStack(test)',
errors: [
{
message: `Wrap lowPriorityWarningWithoutStack() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (potato) {
lowPriorityWarningWithoutStack(test);
}
`,
errors: [
{
message: `Wrap lowPriorityWarningWithoutStack() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (__DEV__ || potato && true) {
warning(test);
}
`,
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (banana && __DEV__ && potato && kitten) {
warning(test);
}
`,
// Technically this code is valid but we prefer
// explicit standalone __DEV__ blocks that stand out.
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (!__DEV__) {
warning(test);
}
`,
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (foo || x && __DEV__) {
warning(test);
}
`,
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (__DEV__) {
} else {
warning(test);
}
`,
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
{
code: `
if (__DEV__) {
} else {
if (__DEV__) {
} else {
warning(test);
}
}
`,
errors: [
{
message: `Wrap warning() in an "if (__DEV__) {}" check`,
},
],
},
],
});

1
scripts/eslint-rules/index.js
View File

@@ -5,5 +5,6 @@ module.exports = {
'no-primitive-constructors': require('./no-primitive-constructors'),
'no-to-warn-dev-within-to-throw': require('./no-to-warn-dev-within-to-throw'),
'warning-and-invariant-args': require('./warning-and-invariant-args'),
'no-production-logging': require('./no-production-logging'),
},
};

72
scripts/eslint-rules/no-production-logging.js Normal file
View File

@@ -0,0 +1,72 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/
'use strict';
const LOGGER_FN_NAMES = [
'warning',
'warningWithoutStack',
'lowPriorityWarning',
'lowPriorityWarningWithoutStack',
];
module.exports = function(context) {
function isInDEVBlock(node) {
let done = false;
while (!done) {
let parent = node.parent;
if (!parent) {
return false;
}
if (
parent.type === 'IfStatement' &&
node === parent.consequent &&
parent.test.type === 'Identifier' &&
// This is intentionally strict so we can
// see blocks of DEV-only code at once.
parent.test.name === '__DEV__'
) {
return true;
}
node = parent;
}
}
function report(node) {
context.report({
node: node,
message: `Wrap {{identifier}}() in an "if (__DEV__) {}" check`,
data: {
identifier: node.callee.name,
},
fix: function(fixer) {
return [
fixer.insertTextBefore(node.parent, `if (__DEV__) {`),
fixer.insertTextAfter(node.parent, '}'),
];
},
});
}
const isLoggerFunctionName = name => LOGGER_FN_NAMES.includes(name);
return {
meta: {
fixable: 'code',
},
CallExpression: function(node) {
if (!isLoggerFunctionName(node.callee.name)) {
return;
}
if (!isInDEVBlock(node)) {
report(node);
}
},
};
};

2
scripts/jest/preprocessor.js
View File

@@ -17,7 +17,7 @@ const pathToBabelPluginDevWithCode = require.resolve(
'../error-codes/transform-error-messages'
);
const pathToBabelPluginWrapWarning = require.resolve(
'../babel/wrap-warning-with-env-check'
'../babel/lift-warning-conditional-argument'
);
const pathToBabelPluginAsyncToGenerator = require.resolve(
'@babel/plugin-transform-async-to-generator'

6
scripts/rollup/build.js
View File

@@ -125,7 +125,7 @@ function getBabelConfig(updateBabelOptions, bundleType, filename) {
// Minify invariant messages
require('../error-codes/transform-error-messages'),
// Wrap warning() calls in a __DEV__ check so they are stripped from production.
require('../babel/wrap-warning-with-env-check'),
require('../babel/lift-warning-conditional-argument'),
]),
});
case RN_OSS_DEV:
@@ -142,7 +142,7 @@ function getBabelConfig(updateBabelOptions, bundleType, filename) {
{noMinify: true},
],
// Wrap warning() calls in a __DEV__ check so they are stripped from production.
require('../babel/wrap-warning-with-env-check'),
require('../babel/lift-warning-conditional-argument'),
]),
});
case UMD_DEV:
@@ -158,7 +158,7 @@ function getBabelConfig(updateBabelOptions, bundleType, filename) {
// Minify invariant messages
require('../error-codes/transform-error-messages'),
// Wrap warning() calls in a __DEV__ check so they are stripped from production.
require('../babel/wrap-warning-with-env-check'),
require('../babel/lift-warning-conditional-argument'),
]),
});
default: