-
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?
Conversation
Bundle size report
Check out the code infra dashboard for more information about this PR. |
Implements a new ESLint rule that enforces certain function calls (warnOnce, warn, checkSlot) to be wrapped with `if (process.env.NODE_ENV !== 'production')` conditional to prevent them from ending up in production bundles. The rule: - Detects calls to specified functions (configurable via options) - Ensures they are wrapped with the correct production check - Catches incorrect conditions (e.g., === instead of !==) - Ignores calls in else blocks of production checks - Includes comprehensive test coverage Co-authored-by: brijeshb42 <[email protected]>
Co-authored-by: brijeshb42 <[email protected]>
| * checkSlot(key, overrides[k]); // Will trigger error | ||
| * | ||
| * @example | ||
| * // Invalid - wrong condition (=== instead of !==) |
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.
// valid
if (process.env.NODE_ENV !== 'production') {}
// valid
if (process.env.NODE_ENV === 'production') {}
// invalid (only compare with production, write !== 'production' instead)
if (process.env.NODE_ENV === 'development') {}
// invalid (only consider production vs.test)
if (process.env.NODE_ENV !== 'test') {}
// invalid (not statically analyzable construct)
const env = 'production';
if (process.env.NODE_ENV !== env) {}
// invalid (not statically analyzable construct)
if (foo(process.env.NODE_ENV)) {}
// i.e. anything outside a comparison binary operationThere 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:// valid if (process.env.NODE_ENV !== 'production') {} // valid if (process.env.NODE_ENV === 'production') {} // invalid (only compare with production, write !== 'production' instead) if (process.env.NODE_ENV === 'development') {} // invalid (only consider production vs.test) if (process.env.NODE_ENV !== 'test') {} // invalid (not statically analyzable construct) const env = 'production'; if (process.env.NODE_ENV !== env) {} // invalid (not statically analyzable construct) if (foo(process.env.NODE_ENV)) {} // i.e. anything outside a comparison binary operation
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.
- Allow both `process.env.NODE_ENV === 'production'` and `!== 'production'` as valid
- Only allow comparison with literal 'production' string
- Reject comparisons with other environment values ('development', 'test', etc.)
- Reject non-statically analyzable constructs (e.g., comparing with variables)
- Fix circular reference issue in containsProcessEnvNodeEnv function
- Update tests to reflect new behavior (18 tests total)
Co-authored-by: brijeshb42 <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
- Simplified `require-dev-wrapper`: Only checks if functions are wrapped with any production check - New `consistent-production-guard`: Enforces production guard patterns (only 'production' literal, no variables, etc.) - Updated tests for require-dev-wrapper (18 tests) - Added comprehensive tests for consistent-production-guard (13 tests) - Both rules registered in plugin index - All 1388 tests passing Co-authored-by: Janpot <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
packages/code-infra/src/eslint/material-ui/rules/consistent-production-guard.mjs
Outdated
Show resolved
Hide resolved
packages/code-infra/src/eslint/material-ui/rules/require-dev-wrapper.mjs
Outdated
Show resolved
Hide resolved
- Created shared nodeEnvUtils.mjs with isProcessEnvNodeEnv and isLiteral helpers - Refactored consistent-production-guard to use checkGuard helper (DRY) - Fixed require-dev-wrapper to handle else blocks correctly (valid when if checks for production) - Added test for else block scenario - Simplified error messages by removing nonStaticComparison (combined into invalidComparison) - All 1389 tests passing Co-authored-by: Janpot <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
packages/code-infra/src/eslint/material-ui/rules/require-dev-wrapper.mjs
Show resolved
Hide resolved
- Changed signature from (left, right, operator, value) to (binaryExpression, operator, value) - Moved logic for checking any NODE_ENV comparison into isNodeEnvComparison when operator is !== - Removed redundant second check in require-dev-wrapper - Simplified code and improved encapsulation - All 1389 tests still passing Co-authored-by: Janpot <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
packages/code-infra/src/eslint/material-ui/rules/nodeEnvUtils.mjs
Outdated
Show resolved
Hide resolved
packages/code-infra/src/eslint/material-ui/rules/require-dev-wrapper.mjs
Outdated
Show resolved
Hide resolved
…etection - Moved isNodeEnvComparison from nodeEnvUtils.mjs to require-dev-wrapper.mjs (not reused) - Fixed logic: for !== operator, also allow === with any literal value - Simplified branch detection using currentChild variable instead of inner loop - All 1389 tests still passing Co-authored-by: Janpot <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| if ( | ||
| operator === '!==' && | ||
| binaryExpression.operator === '===' && | ||
| ((isProcessEnvNodeEnv(left) && right.type === 'Literal') || |
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 is it you not getting it or is it me?
"not equals a certain value" is equivalent to "equals not that value"
| ((isProcessEnvNodeEnv(left) && right.type === 'Literal') || | |
| ((isProcessEnvNodeEnv(left) && right.type === 'Literal' && right.value !== value) || |
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.
You're absolutely right! Fixed the logic to check right.value !== value to ensure we only accept === comparisons with values that are NOT 'production'.
Fixed in commit ba214aa.
| } | ||
|
|
||
| // For !== operator in consequent, also allow !== with any other value | ||
| if ( |
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 remove this, jesus
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.
Removed the unnecessary !== with any value check. The rule now only accepts tree-shakeable patterns.
Fixed in commit ba214aa.
| } | ||
| `, | ||
| }, | ||
| // Should pass: Function wrapped with === production check |
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 should not pass, it needs to check that it gets tree shaken away in production
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.
Moved to invalid tests. Code in the then block of === 'production' runs IN production, so it doesn't tree-shake away.
Fixed in commit ba214aa.
| } | ||
| `, | ||
| }, | ||
| // Should pass: Function wrapped with !== 'test' check (still wrapped) |
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 Wrong, again, when node env is production, the code shoeuld tree shake away
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.
Removed from valid tests. !== 'test' doesn't reliably tree-shake in production.
Fixed in commit ba214aa.
| } | ||
| `, | ||
| }, | ||
| // Should pass: Function wrapped with non-static check (still wrapped) |
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 wrong, irrelevant, remove test
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.
Removed. Non-static comparisons don't tree-shake.
Fixed in commit ba214aa.
| } | ||
| `, | ||
| }, | ||
| // Should pass: warn wrapped correctly with === |
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 wrong, should not pass, this would keep the code in production
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.
Moved to invalid tests. This pattern would execute the dev code IN production.
Fixed in commit ba214aa.
| }, | ||
| ], | ||
| }, | ||
| // Should fail: Inside else block of production check (!== 'production') |
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 great, also add the other variant when its in the then block of === 'production'
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.
Added duplicate test case - there were already two tests with the same code in the valid section. The else block test covers this pattern.
Fixed in commit ba214aa.
- Changed logic to only accept patterns that tree-shake in production - Consequent: only !== 'production' or === with non-production values - Alternate (else): only === 'production' - Removed invalid test cases that don't tree-shake - Added invalid tests for patterns that keep code in production - All 1388 tests passing Co-authored-by: Janpot <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
Pull Request Overview
This PR introduces two complementary ESLint rules to enforce tree-shakeable production guards and prevent development-only code from ending up in production bundles. The implementation ensures that specific development functions (like warnOnce, warn, and checkSlot) are properly wrapped with production environment checks that will be eliminated during the production build process.
Key Changes:
- Added
require-dev-wrapperrule to enforce tree-shakeable production guards for specific development functions - Added
consistent-production-guardrule to ensure allprocess.env.NODE_ENVchecks follow best practices - Created shared utility functions in
nodeEnvUtils.mjsfor consistent environment check validation across both rules
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/code-infra/src/eslint/material-ui/rules/require-dev-wrapper.mjs | Core logic for detecting unwrapped dev functions and validating tree-shakeable patterns |
| packages/code-infra/src/eslint/material-ui/rules/require-dev-wrapper.test.mjs | Comprehensive test suite with 18 test cases covering valid/invalid patterns |
| packages/code-infra/src/eslint/material-ui/rules/consistent-production-guard.mjs | Enforces consistent production guard patterns across all NODE_ENV checks |
| packages/code-infra/src/eslint/material-ui/rules/consistent-production-guard.test.mjs | Test suite with 13 test cases validating guard pattern enforcement |
| packages/code-infra/src/eslint/material-ui/rules/nodeEnvUtils.mjs | Shared utilities for AST node validation related to process.env.NODE_ENV |
| packages/code-infra/src/eslint/material-ui/index.mjs | Registration of both new rules in the material-ui ESLint plugin |
| if (current.type === 'IfStatement') { | ||
| // Determine which branch we're in | ||
| const isInConsequent = current.consequent === currentChild; | ||
| const isInAlternate = current.alternate === currentChild; | ||
|
|
||
| // Skip if not in a branch | ||
| if (isInConsequent || isInAlternate) { |
Copilot
AI
Oct 15, 2025
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.
The condition at line 108 could fail to detect the alternate branch when the alternate is a block statement. When currentChild is a BlockStatement that wraps the actual alternate branch, the reference equality check current.alternate === currentChild will be true. However, the code should also check if currentChild is contained within the alternate branch for nested cases. Consider using a helper function to check if a node is contained within a branch to handle both direct and nested cases.
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 you come up with a code snippet to prove your point in your previous 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.
The current implementation actually handles this correctly. In JavaScript AST, when you have:
if (process.env.NODE_ENV === 'production') {
// some code
} else {
warnOnce(); // our target node
}The alternate property of the IfStatement directly points to the BlockStatement containing the else clause. As we traverse up from warnOnce(), currentChild will be the BlockStatement, and current.alternate === currentChild will be true.
For nested if statements:
if (process.env.NODE_ENV === 'production') {
// some code
} else {
if (someCondition) {
warnOnce(); // our target node
}
}When we reach the outer if statement during traversal, currentChild will be the inner IfStatement (not the BlockStatement), so current.alternate === currentChild will still work correctly.
The reference equality check works because we're updating currentChild to be the previous current value in each iteration of the while loop, which maintains the direct parent-child relationship needed for the comparison.
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 you propose a test case for this and add it to the suite?
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.
Added a test case for nested if statements in else blocks. The test verifies that warnOnce inside a nested if statement within the else block of a process.env.NODE_ENV === 'production' check is correctly recognized as wrapped. All 19 tests pass.
Fixed in commit above.
- Added test to verify branch detection works correctly with nested if statements - Tests scenario where warnOnce is in a nested if inside else block of production check - All 19 tests passing including the new test case Co-authored-by: Janpot <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
require-dev-wrapperImplementation Summary
Created two ESLint rules to enforce production guards with shared utilities:
Shared Utilities (
nodeEnvUtils.mjs)isProcessEnvNodeEnv(node): Checks if a node isprocess.env.NODE_ENVisLiteral(node, value): Checks if a node is a Literal with specific value1.
material-ui/require-dev-wrapperwarnOnce,warn,checkSlot) must be wrapped with tree-shakeable production checksif (process.env.NODE_ENV !== 'production') { devCode() }- tree-shakesif (process.env.NODE_ENV === 'development') { devCode() }- tree-shakesif (process.env.NODE_ENV === 'production') {} else { devCode() }- tree-shakesif (process.env.NODE_ENV === 'production') { devCode() }- does NOT tree-shake!if (process.env.NODE_ENV !== 'test') { devCode() }- does NOT tree-shake reliablycurrentChildvariable for efficient branch detection2.
material-ui/consistent-production-guardprocess.env.NODE_ENV'production'process.env.NODE_ENVis only used in binary comparisons with===or!==checkGuardhelper to DRY up left/right comparison logicThese two rules work together but can be enabled independently:
require-dev-wrapperensures critical dev functions are guardedconsistent-production-guardensures all production guards follow best practicesThe rules help prevent development-only utilities from adding weight to production bundles, addressing issue #207.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.