Skip to content

Commit

Permalink
feat(eslint-config): new rulе no-modifying-ref-on-render
Browse files Browse the repository at this point in the history
  • Loading branch information
SeanSilke committed Mar 13, 2024
1 parent 9004111 commit c8c78ab
Show file tree
Hide file tree
Showing 5 changed files with 213 additions and 73 deletions.
70 changes: 70 additions & 0 deletions packages/eslint-config/rules/helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// The next function is taken form here
// https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
/**
* Gets the static name of a function AST node. For function declarations it is
* easy. For anonymous function expressions it is much harder. If you search for
* `IsAnonymousFunctionDefinition()` in the ECMAScript spec you'll find places
* where JS gives anonymous function expressions names. We roughly detect the
* same AST nodes with some exceptions to better fit our use case.
*/

function getFunctionName(node) {
if (node.type === 'FunctionDeclaration' || (node.type === 'FunctionExpression' && node.id)) {
// function useHook() {}
// const whatever = function useHook() {};
//
// Function declaration or function expression names win over any
// assignment statements or other renames.
return node.id;
}

if (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') {
if (node.parent.type === 'VariableDeclarator' && node.parent.init === node) {
// const useHook = () => {};
return node.parent.id;
}

if (node.parent.type === 'AssignmentExpression' && node.parent.right === node && node.parent.operator === '=') {
// useHook = () => {};
return node.parent.left;
}

if (node.parent.type === 'Property' && node.parent.value === node && !node.parent.computed) {
// {useHook: () => {}}
// {useHook() {}}
return node.parent.key;

// NOTE: We could also support `ClassProperty` and `MethodDefinition`
// here to be pedantic. However, hooks in a class are an anti-pattern. So
// we don't allow it to error early.
//
// class {useHook = () => {}}
// class {useHook() {}}
}

if (node.parent.type === 'AssignmentPattern' && node.parent.right === node && !node.parent.computed) {
// const {useHook = () => {}} = {};
// ({useHook = () => {}} = {});
//
// Kinda clowny, but we'd said we'd follow spec convention for
// `IsAnonymousFunctionDefinition()` usage.
return node.parent.left;
}

return undefined;
}

return undefined;
}

/**
* Checks if the node is a React component name. React component names must
* always start with an uppercase letter.
*/

function isComponentName(node) {
return node.type === 'Identifier' && /^[A-Z]/.test(node.name);
}

exports.isComponentName = isComponentName;
exports.getFunctionName = getFunctionName;
4 changes: 3 additions & 1 deletion packages/eslint-config/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ const projectName = 'salute-rules';
const fs = require('fs');
const path = require('path');

const ruleFiles = fs.readdirSync(__dirname).filter((file) => file !== 'index.js' && !file.endsWith('test.js'));
const ruleFiles = fs
.readdirSync(__dirname)
.filter((file) => file !== 'index.js' && file !== 'helpers.js' && !file.endsWith('test.js'));

const configs = {
all: {
Expand Down
85 changes: 85 additions & 0 deletions packages/eslint-config/rules/no-modifying-ref-on-render.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
const { isComponentName, getFunctionName } = require('./helpers');
// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------

function reportError(context, node) {
context.report({
node,
message: 'Do not modifying ref on render',
});
}

module.exports = {
meta: {
type: null, // `problem`, `suggestion`, or `layout`
docs: {
description: 'Do not modifying ref on render',
recommended: true,
url: 'https://react.dev/reference/react/useRef#caveats',
},
fixable: null, // Or `code` or `whitespace`
schema: [], // Add a schema if the rule has options
},

create: (context) => {
let isInUseEffect = false;
let currentComponent;
let currentComponentRefNames = [];

return {
CallExpression(node) {
// Checking if the invoked function is useRef
if (node.callee.type === 'Identifier' && node.callee.name === 'useRef') {
const isDeclaration =
node.parent &&
node.parent.type === 'VariableDeclarator' &&
node.parent.id.type === 'Identifier';

if (!isDeclaration) {
return;
}

const refName = node.parent.id.name;
currentComponentRefNames.push(refName);
}

// Checking if the invoked function is useEffect
if (node.callee.type === 'Identifier' && node.callee.name === 'useEffect') {
isInUseEffect = true;
}
},
'CallExpression:exit': function (node) {
if (node.callee.type === 'Identifier' && node.callee.name === 'useEffect') {
isInUseEffect = false;
}
},

':function': function (node) {
const name = getFunctionName(node);
currFunctionName = name && name.name;

if (name && isComponentName(name)) {
currentComponent = node;
}
},

':function:exit': function (node) {
if (node === currentComponent) {
currentComponent = null;
currentComponentRefNames = [];
}
},

'AssignmentExpression > MemberExpression': function (node) {
if (
currentComponentRefNames.includes(node.object.name) &&
node.property.name === 'current' &&
!isInUseEffect
) {
reportError(context, node);
}
},
};
},
};
50 changes: 50 additions & 0 deletions packages/eslint-config/rules/no-modifying-ref-on-render.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
const { RuleTester } = require('eslint');

const rule = require('./no-modifying-ref-on-render');

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 2018,
sourceType: 'module',
ecmaFeatures: {
jsx: true,
},
},
});

// Use only: true, too run one test only

const tests = {
valid: [
{
code: `
const MyComponent = (props) => {
const ref = useRef(props);
useEffect(() => {
ref.current = props;
})
return <div>text</div>;
}`,
},
],
invalid: [
{
code: `
const MyComponent = (props) => {
const ref = useRef(props);
ref.current = props;
return <div>text</div>;
}`,
errors: [errorMsg()],
},
],
};

function errorMsg() {
return { message: 'Do not modifying ref on render' };
}

ruleTester.run('no-modifying-ref-on-render', rule, tests);
77 changes: 5 additions & 72 deletions packages/eslint-config/rules/no-redundant-commit.js
Original file line number Diff line number Diff line change
@@ -1,70 +1,4 @@
// The next function is taken form here
// https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
/**
* Gets the static name of a function AST node. For function declarations it is
* easy. For anonymous function expressions it is much harder. If you search for
* `IsAnonymousFunctionDefinition()` in the ECMAScript spec you'll find places
* where JS gives anonymous function expressions names. We roughly detect the
* same AST nodes with some exceptions to better fit our use case.
*/

function getFunctionName(node) {
if (node.type === 'FunctionDeclaration' || (node.type === 'FunctionExpression' && node.id)) {
// function useHook() {}
// const whatever = function useHook() {};
//
// Function declaration or function expression names win over any
// assignment statements or other renames.
return node.id;
}

if (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') {
if (node.parent.type === 'VariableDeclarator' && node.parent.init === node) {
// const useHook = () => {};
return node.parent.id;
}

if (node.parent.type === 'AssignmentExpression' && node.parent.right === node && node.parent.operator === '=') {
// useHook = () => {};
return node.parent.left;
}

if (node.parent.type === 'Property' && node.parent.value === node && !node.parent.computed) {
// {useHook: () => {}}
// {useHook() {}}
return node.parent.key;

// NOTE: We could also support `ClassProperty` and `MethodDefinition`
// here to be pedantic. However, hooks in a class are an anti-pattern. So
// we don't allow it to error early.
//
// class {useHook = () => {}}
// class {useHook() {}}
}

if (node.parent.type === 'AssignmentPattern' && node.parent.right === node && !node.parent.computed) {
// const {useHook = () => {}} = {};
// ({useHook = () => {}} = {});
//
// Kinda clowny, but we'd said we'd follow spec convention for
// `IsAnonymousFunctionDefinition()` usage.
return node.parent.left;
}

return undefined;
}

return undefined;
}

/**
* Checks if the node is a React component name. React component names must
* always start with an uppercase letter.
*/

function isComponentName(node) {
return node.type === 'Identifier' && /^[A-Z]/.test(node.name);
}
const { isComponentName, getFunctionName } = require('./helpers');

// taken from https://github.com/eslint-community/eslint-plugin-promise/tree/main
function hasPromiseCallback(node) {
Expand Down Expand Up @@ -111,20 +45,19 @@ module.exports = {
currentComponentStateSettersNames.includes(node.callee.name) &&
!isInAsync
) {

if (
isInUseEffect &&
callExpressionNestingLevel == 2 && // function is called on the level of useEffect(()=> {'around here and not deeper' }, [])
callExpressionNestingLevel == 2 && // function is called on the level of useEffect(()=> {'around here and not deeper' }, [])
UseEffectFunctionsNestingLevel == 1
) {
) {
context.report({
node,
message: 'Avoid using synchronous state setters within effects',
});
} else { // call is inside new function
} else {
// call is inside new function
currentComponentStateSettersNames.push(currFunctionName);
}

}

// Checking if the invoked function is useEffect
Expand Down

0 comments on commit c8c78ab

Please sign in to comment.