fix(no-export): detect more CommonJS export assignment patterns#1977
fix(no-export): detect more CommonJS export assignment patterns#1977eryue0220 wants to merge 8 commits into
Conversation
Require the exact exports property name instead of matching export typos, and ignore assignments when module or exports refers to a local binding. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a test case for module[exports] assignments when exports refers to the global CommonJS binding, satisfying the 100% coverage requirement. Co-authored-by: Cursor <cursoragent@cursor.com>
Refactor assignment detection to walk the full member chain and report module["exports"], exports =, exports.foo =, and nested module.exports assignments that were previously missed. Co-authored-by: Cursor <cursoragent@cursor.com>
| resolveScope, | ||
| } from './utils'; | ||
|
|
||
| const isGlobalModuleIdentifier = ( |
There was a problem hiding this comment.
I think it would make more sense to have a single isGlobalIdentifier function that you can pass the identifier name into rather than two slightly different functions
| { | ||
| code: 'module.export.invalid = function() {}; ; test("a test", () => { expect(1).toBe(1);});', | ||
| errors: [{ endColumn: 22, column: 1, messageId: 'unexpectedExport' }], | ||
| code: 'module[exports] = function() {}; test("a test", () => { expect(1).toBe(1);});', |
There was a problem hiding this comment.
we cannot assume that a variable has a specific value, and frankly I don't think there's any valid reason for someone to do this so it's fine to not support it.
on the other hand, there's really no reason to do any other assignment to module so it might be simpler still to just check for an assignment to anything on global module - that would simplify our logic to:
- is this a global?
- is this either
moduleorexports? - is this on the left hand side of an assignment?
There was a problem hiding this comment.
Done — I've simplified the logic along those lines and dropped the module[exports] test case.
The check now boils down to:
- Is this a global? —
isGlobalIdentifierusesresolveScope(...) === null, so locally shadowedmodule/exportsbindings are ignored. - Is this either
moduleorexports? — for a direct assignment we check the LHS identifier; for member expressions we walk to the root identifier and check whether it's globalmoduleorexports(so any assignment on globalmoduleis caught, not justmodule.exports). - Is this on the left-hand side of an assignment? — we only handle this in the
AssignmentExpressionvisitor, inspectingnode.left.
No longer special-case computed properties like module[exports] — I agree there's no good reason to support that pattern, and the simplified root-identifier check covers the cases we care about without assuming anything about variable values.
G-Rath
left a comment
G-Rath
left a comment
There was a problem hiding this comment.
can we now pull out adding support for exports? that feels like it'll take a huge chunk out of this diff.
also can we not drop support for export for now? I know it's technically not valid but it's been there from the start and for now it'll also keep the diff down
Okay updated. |
No you have not - I'm wanting a PR that just adds support for |
another pr about |
2 participants
Based on #1976, fixes false negatives in jest/no-export for CommonJS export assignments in test files.
Nested assignments such as module.exports.foo.bar = … — only the outermost member was checked