Skip to content

Commit

Permalink
Fixes eslint warning when node type is ChainExpression (#19680)
Browse files Browse the repository at this point in the history 
* Add babel parser which supports ChainExpression

* Add and fix tests for new babel eslint parser

* extract function to mark node

* refactor for compatibility with eslint v7.7.0+

* Update eslint to v7.7.0
Update hook test since eslint now supports nullish coalescing
  • Loading branch information
Pascal Fong Kye authored Aug 29, 2020
1 parent a8500be commit 1396e4a
Show file tree
Hide file tree
Showing 6 changed files with 249 additions and 275 deletions.
3 changes: 2 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"@babel/cli": "^7.10.5",
"@babel/code-frame": "^7.10.4",
"@babel/core": "^7.11.1",
"@babel/eslint-parser": "^7.11.4",
"@babel/helper-module-imports": "^7.10.4",
"@babel/parser": "^7.11.3",
"@babel/plugin-external-helpers": "^7.10.4",
Expand Down Expand Up @@ -46,7 +47,7 @@
"create-react-class": "^15.6.3",
"danger": "^9.2.10",
"error-stack-parser": "^2.0.6",
"eslint": "^7.0.0",
"eslint": "^7.7.0",
"eslint-config-fbjs": "^1.1.1",
"eslint-config-prettier": "^6.9.0",
"eslint-plugin-babel": "^5.3.0",
Expand Down
5 changes: 5 additions & 0 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7972,6 +7972,11 @@ new ESLintTester({
parserOptions,
}).run('react-hooks', ReactHooksESLintRule, tests);

new ESLintTester({
parser: require.resolve('@babel/eslint-parser'),
parserOptions,
}).run('react-hooks', ReactHooksESLintRule, tests);

new ESLintTester({
parser: require.resolve('@typescript-eslint/parser'),
parserOptions,
Expand Down
4 changes: 1 addition & 3 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,9 +752,7 @@ const tests = {
errors: [
conditionalError('useState'),
conditionalError('useState'),
// TODO: ideally this *should* warn, but ESLint
// doesn't plan full support for ?? until it advances.
// conditionalError('useState'),
conditionalError('useState'),
],
},
{
Expand Down
54 changes: 33 additions & 21 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,9 +814,10 @@ export default {
let maybeID = declaredDependencyNode;
while (
maybeID.type === 'MemberExpression' ||
maybeID.type === 'OptionalMemberExpression'
maybeID.type === 'OptionalMemberExpression' ||
maybeID.type === 'ChainExpression'
) {
maybeID = maybeID.object;
maybeID = maybeID.object || maybeID.expression.object;
}
const isDeclaredInComponent = !componentScope.through.some(
ref => ref.identifier === maybeID,
Expand Down Expand Up @@ -1590,6 +1591,27 @@ function getDependency(node) {
}
}

/**
* Mark a node as either optional or required.
* Note: If the node argument is an OptionalMemberExpression, it doesn't necessarily mean it is optional.
* It just means there is an optional member somewhere inside.
* This particular node might still represent a required member, so check .optional field.
*/
function markNode(node, optionalChains, result) {
if (optionalChains) {
if (node.optional) {
// We only want to consider it optional if *all* usages were optional.
if (!optionalChains.has(result)) {
// Mark as (maybe) optional. If there's a required usage, this will be overridden.
optionalChains.set(result, true);
}
} else {
// Mark as required.
optionalChains.set(result, false);
}
}
}

/**
* Assuming () means the passed node.
* (foo) -> 'foo'
Expand All @@ -1609,30 +1631,20 @@ function analyzePropertyChain(node, optionalChains) {
const object = analyzePropertyChain(node.object, optionalChains);
const property = analyzePropertyChain(node.property, null);
const result = `${object}.${property}`;
if (optionalChains) {
// Mark as required.
optionalChains.set(result, false);
}
markNode(node, optionalChains, result);
return result;
} else if (node.type === 'OptionalMemberExpression' && !node.computed) {
const object = analyzePropertyChain(node.object, optionalChains);
const property = analyzePropertyChain(node.property, null);
const result = `${object}.${property}`;
if (optionalChains) {
// Note: OptionalMemberExpression doesn't necessarily mean this node is optional.
// It just means there is an optional member somewhere inside.
// This particular node might still represent a required member, so check .optional field.
if (node.optional) {
// We only want to consider it optional if *all* usages were optional.
if (!optionalChains.has(result)) {
// Mark as (maybe) optional. If there's a required usage, this will be overridden.
optionalChains.set(result, true);
}
} else {
// Mark as required.
optionalChains.set(result, false);
}
}
markNode(node, optionalChains, result);
return result;
} else if (node.type === 'ChainExpression' && !node.computed) {
const expression = node.expression;
const object = analyzePropertyChain(expression.object, optionalChains);
const property = analyzePropertyChain(expression.property, null);
const result = `${object}.${property}`;
markNode(expression, optionalChains, result);
return result;
} else {
throw new Error(`Unsupported node type: ${node.type}`);
Expand Down
128 changes: 65 additions & 63 deletions scripts/eslint-rules/no-production-logging.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,75 +9,77 @@

'use strict';

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;
module.exports = {
meta: {
fixable: 'code',
},
create: 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;
}
node = parent;
}
}

function reportWrapInDEV(node) {
context.report({
node: node,
message: `Wrap console.{{identifier}}() in an "if (__DEV__) {}" check`,
data: {
identifier: node.property.name,
},
fix: function(fixer) {
return [
fixer.insertTextBefore(node.parent, `if (__DEV__) {`),
fixer.insertTextAfter(node.parent, '}'),
];
},
});
}
function reportWrapInDEV(node) {
context.report({
node: node,
message: `Wrap console.{{identifier}}() in an "if (__DEV__) {}" check`,
data: {
identifier: node.property.name,
},
fix: function(fixer) {
return [
fixer.insertTextBefore(node.parent, `if (__DEV__) {`),
fixer.insertTextAfter(node.parent, '}'),
];
},
});
}

function reportUnexpectedConsole(node) {
context.report({
node: node,
message: `Unexpected use of console`,
});
}
function reportUnexpectedConsole(node) {
context.report({
node: node,
message: `Unexpected use of console`,
});
}

return {
meta: {
fixable: 'code',
},
MemberExpression: function(node) {
if (
node.object.type === 'Identifier' &&
node.object.name === 'console' &&
node.property.type === 'Identifier'
) {
switch (node.property.name) {
case 'error':
case 'warn': {
if (!isInDEVBlock(node)) {
reportWrapInDEV(node);
return {
MemberExpression: function(node) {
if (
node.object.type === 'Identifier' &&
node.object.name === 'console' &&
node.property.type === 'Identifier'
) {
switch (node.property.name) {
case 'error':
case 'warn': {
if (!isInDEVBlock(node)) {
reportWrapInDEV(node);
}
break;
}
default: {
reportUnexpectedConsole(node);
break;
}
break;
}
default: {
reportUnexpectedConsole(node);
break;
}
}
}
},
};
},
};
},
};
Loading

0 comments on commit 1396e4a

Please sign in to comment.