-
Notifications
You must be signed in to change notification settings - Fork 28
[code-infra] Add ESLint rules to enforce tree-shakeable production guards for dev-only functions #794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[code-infra] Add ESLint rules to enforce tree-shakeable production guards for dev-only functions #794
Changes from 3 commits
1a47091
39925a9
cd90e72
911cf1b
a98e23a
3650dff
c14b74f
1942d5a
651aa6a
7937026
7816981
ba214aa
ebe83ed
a476694
25f9303
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| /** | ||
| * ESLint rule that enforces certain function calls to be wrapped with | ||
| * a production check to prevent them from ending up in production bundles. | ||
| * | ||
| * @example | ||
| * // Valid - function wrapped with production check | ||
| * if (process.env.NODE_ENV !== 'production') { | ||
| * checkSlot(key, overrides[k]); | ||
| * } | ||
| * | ||
| * @example | ||
| * // Invalid - function not wrapped | ||
| * checkSlot(key, overrides[k]); // Will trigger error | ||
| * | ||
| * @example | ||
| * // Invalid - wrong condition (=== instead of !==) | ||
| * if (process.env.NODE_ENV === 'production') { | ||
| * checkSlot(key, overrides[k]); // Will trigger error | ||
| * } | ||
| * | ||
| * @example | ||
| * // Usage in ESLint config | ||
| * { | ||
| * rules: { | ||
| * 'material-ui/require-dev-wrapper': ['error', { | ||
| * functionNames: ['warnOnce', 'warn', 'checkSlot'] | ||
| * }] | ||
| * } | ||
| * } | ||
| * | ||
| * @type {import('eslint').Rule.RuleModule} | ||
| */ | ||
| const rule = { | ||
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: | ||
| 'Enforce that certain function calls are wrapped with a production check to prevent them from ending up in production bundles', | ||
| }, | ||
| messages: { | ||
| missingDevWrapper: | ||
| "Function `{{ functionName }}` must be wrapped with `if (process.env.NODE_ENV !== 'production')` to prevent it from ending up in production bundles.", | ||
| wrongCondition: | ||
| "Function `{{ functionName }}` must be wrapped with `if (process.env.NODE_ENV !== 'production')` (not `===`).", | ||
| }, | ||
| schema: [ | ||
| { | ||
| type: 'object', | ||
| properties: { | ||
| functionNames: { | ||
| type: 'array', | ||
| items: { | ||
| type: 'string', | ||
| }, | ||
| default: ['warnOnce', 'warn', 'checkSlot'], | ||
| }, | ||
| }, | ||
| additionalProperties: false, | ||
| }, | ||
| ], | ||
| }, | ||
| create(context) { | ||
| const options = context.options[0] || {}; | ||
| const functionNames = options.functionNames || ['warnOnce', 'warn', 'checkSlot']; | ||
|
|
||
| /** | ||
| * Checks if a node is wrapped in a production check conditional | ||
| * @param {import('estree').Node & import('eslint').Rule.NodeParentExtension} node | ||
| * @returns {{ wrapped: boolean; wrongCondition: boolean }} | ||
| */ | ||
| function isWrappedInProductionCheck(node) { | ||
Janpot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let current = node.parent; | ||
|
|
||
| while (current) { | ||
| // Check if we're inside an if statement | ||
| if (current.type === 'IfStatement') { | ||
| const test = current.test; | ||
|
|
||
| // Make sure we're in the consequent (then) block, not the alternate (else) block | ||
| let isInConsequent = false; | ||
| let temp = node; | ||
| while (temp && temp !== current) { | ||
Janpot marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (temp === current.consequent) { | ||
| isInConsequent = true; | ||
| break; | ||
| } | ||
| temp = temp.parent; | ||
| } | ||
|
|
||
| if (!isInConsequent) { | ||
Janpot marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Continue looking up the tree if we're in the alternate branch | ||
| current = current.parent; | ||
| continue; | ||
| } | ||
|
|
||
| // Check for: process.env.NODE_ENV !== 'production' | ||
| if ( | ||
| test.type === 'BinaryExpression' && | ||
| test.operator === '!==' && | ||
| isProcessEnvNodeEnv(test.left) && | ||
| test.right.type === 'Literal' && | ||
| test.right.value === 'production' | ||
| ) { | ||
| return { wrapped: true, wrongCondition: false }; | ||
| } | ||
|
|
||
| // Check for wrong condition: process.env.NODE_ENV === 'production' | ||
| if ( | ||
|
||
| test.type === 'BinaryExpression' && | ||
| test.operator === '===' && | ||
| isProcessEnvNodeEnv(test.left) && | ||
| test.right.type === 'Literal' && | ||
| test.right.value === 'production' | ||
| ) { | ||
| return { wrapped: true, wrongCondition: true }; | ||
| } | ||
| } | ||
|
|
||
| current = current.parent; | ||
| } | ||
|
|
||
| return { wrapped: false, wrongCondition: false }; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a node is process.env.NODE_ENV | ||
| * @param {import('estree').Node} node | ||
| * @returns {boolean} | ||
| */ | ||
| function isProcessEnvNodeEnv(node) { | ||
| return ( | ||
| node.type === 'MemberExpression' && | ||
| node.object.type === 'MemberExpression' && | ||
| node.object.object.type === 'Identifier' && | ||
| node.object.object.name === 'process' && | ||
| node.object.property.type === 'Identifier' && | ||
| node.object.property.name === 'env' && | ||
| node.property.type === 'Identifier' && | ||
| node.property.name === 'NODE_ENV' | ||
| ); | ||
| } | ||
|
|
||
| return { | ||
| CallExpression(node) { | ||
| // Check if the callee is one of the restricted function names | ||
| if (node.callee.type === 'Identifier' && functionNames.includes(node.callee.name)) { | ||
| const { wrapped, wrongCondition } = isWrappedInProductionCheck(node); | ||
|
|
||
| if (!wrapped) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'missingDevWrapper', | ||
| data: { | ||
| functionName: node.callee.name, | ||
| }, | ||
| }); | ||
| } else if (wrongCondition) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'wrongCondition', | ||
| data: { | ||
| functionName: node.callee.name, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
|
Check failure on line 169 in packages/code-infra/src/eslint/material-ui/rules/require-dev-wrapper.mjs
|
||
| }; | ||
|
|
||
| export default rule; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| import eslint from 'eslint'; | ||
| import parser from '@typescript-eslint/parser'; | ||
| import rule from './require-dev-wrapper.mjs'; | ||
|
|
||
| const ruleTester = new eslint.RuleTester({ | ||
| languageOptions: { | ||
| parser, | ||
| }, | ||
| }); | ||
|
|
||
| ruleTester.run('require-dev-wrapper', rule, { | ||
| valid: [ | ||
| // Should pass: Function wrapped with correct production check | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| checkSlot(key, overrides[k]); | ||
| } | ||
| `, | ||
| }, | ||
| // Should pass: Function wrapped in a for loop inside production check | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| for (key in slots) { | ||
| checkSlot(key, overrides[k]); | ||
| } | ||
| } | ||
| `, | ||
| }, | ||
| // Should pass: Other functions not in the list | ||
| { | ||
| code: ` | ||
| otherFunction('hello'); | ||
| `, | ||
| }, | ||
| // Should pass: warnOnce wrapped correctly | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| warnOnce('Some warning message'); | ||
| } | ||
| `, | ||
| }, | ||
| // Should pass: warn wrapped correctly | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| warn('Some warning message'); | ||
| } | ||
| `, | ||
| }, | ||
| // Should pass: Multiple statements in production check | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| const message = 'Warning'; | ||
| warn(message); | ||
| checkSlot(key, value); | ||
| } | ||
| `, | ||
| }, | ||
| // Should pass: Nested blocks | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| if (someCondition) { | ||
| warnOnce('nested warning'); | ||
| } | ||
| } | ||
| `, | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| // Should fail: checkSlot without production check | ||
| { | ||
| code: ` | ||
| checkSlot(key, overrides[k]); | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'checkSlot' }, | ||
| }, | ||
| ], | ||
| }, | ||
| // Should fail: Wrong condition (=== instead of !==) | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV === 'production') { | ||
| checkSlot(key, overrides[k]); | ||
| } | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'wrongCondition', | ||
| data: { functionName: 'checkSlot' }, | ||
| }, | ||
| ], | ||
| }, | ||
| // Should fail: warnOnce without production check | ||
| { | ||
| code: ` | ||
| warnOnce('Some warning'); | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'warnOnce' }, | ||
| }, | ||
| ], | ||
| }, | ||
| // Should fail: warn without production check | ||
| { | ||
| code: ` | ||
| warn('Some warning'); | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'warn' }, | ||
| }, | ||
| ], | ||
| }, | ||
| // Should fail: Multiple unwrapped calls | ||
| { | ||
| code: ` | ||
| checkSlot(key, value); | ||
| warn('Warning message'); | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'checkSlot' }, | ||
| }, | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'warn' }, | ||
| }, | ||
| ], | ||
| }, | ||
| // Should fail: Inside wrong conditional | ||
| { | ||
| code: ` | ||
| if (someOtherCondition) { | ||
| checkSlot(key, value); | ||
| } | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'checkSlot' }, | ||
| }, | ||
| ], | ||
| }, | ||
| // Should fail: Inside else block of production check | ||
| { | ||
| code: ` | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| // ok | ||
| } else { | ||
| checkSlot(key, value); | ||
| } | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'missingDevWrapper', | ||
| data: { functionName: 'checkSlot' }, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong to me. I believe what we want is to enforce mutual exclusive checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Update the code as per this comment ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the rule to accept both
===and!==comparisons with'production', while rejecting comparisons with other values like'development'or'test', and non-static constructs. Fixed in commit cd90e72.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Can we instead split this into two rules?
require-dev-wrapper: to detect functions that require a production guardconsistent-production-guard: which make sure these rules are followed:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split into two separate rules as requested:
require-dev-wrapper- Detects functions that need production guards (simplified to only check if wrapped)consistent-production-guard- Enforces production guard patterns (only 'production' literal, static checks, etc.)Both rules have comprehensive test coverage and can be enabled independently. Changes in commit 911cf1b.