-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(eslint-plugin): detect node API usage more accurately #7664
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: build/v2
Are you sure you want to change the base?
Changes from all commits
e13b384
94e47d7
69dd8fb
765ce4a
64d00a0
e729fdb
bfd176b
d206cf6
07740ac
63cdda0
8514061
eaaf3ec
275ed8a
cde161b
3e304c3
de415bb
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,5 @@ | ||
--- | ||
'eslint-plugin-qwik': patch | ||
--- | ||
|
||
FIX: eslint-plugin: detect node API usage more accurately |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ import { Rule } from 'eslint'; | |
import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/utils'; | ||
import * as eslint from 'eslint'; // For Scope types | ||
const ISSERVER = 'isServer'; | ||
const GLOBALAPIS = ['process', '__dirname', '__filename', 'module']; | ||
const PRESETNODEAPIS = ['fs', 'os', 'path', 'child_process', 'http', 'https', 'Buffer']; | ||
// Helper function: checks if a node is a descendant of another node | ||
function isNodeDescendantOf(descendantNode, ancestorNode): boolean { | ||
if (!ancestorNode) { | ||
|
@@ -35,18 +37,7 @@ export const scopeUseTask: Rule.RuleModule = { | |
forbiddenApis: { | ||
type: 'array', | ||
items: { type: 'string' }, | ||
default: [ | ||
'process', | ||
'fs', | ||
'os', | ||
'path', | ||
'child_process', | ||
'http', | ||
'https', | ||
'Buffer', | ||
'__dirname', | ||
'__filename', | ||
], | ||
default: PRESETNODEAPIS, | ||
}, | ||
}, | ||
additionalProperties: false, | ||
|
@@ -62,18 +53,7 @@ export const scopeUseTask: Rule.RuleModule = { | |
create(context: Rule.RuleContext): Rule.RuleListener { | ||
const options = context.options[0] || {}; | ||
const forbiddenApis = new Set<string>( | ||
options.forbiddenApis || [ | ||
'process', | ||
'fs', | ||
'os', | ||
'path', | ||
'child_process', | ||
'http', | ||
'https', | ||
'Buffer', | ||
'__dirname', | ||
'__filename', | ||
] | ||
options.forbiddenApis || PRESETNODEAPIS.concat(GLOBALAPIS) | ||
); | ||
const serverGuardIdentifier: string = ISSERVER; | ||
const sourceCode = context.sourceCode; | ||
|
@@ -93,7 +73,6 @@ export const scopeUseTask: Rule.RuleModule = { | |
*/ | ||
function isApiUsageGuarded(apiOrCallNode, functionContextNode): boolean { | ||
let currentParentNode: TSESTree.Node | undefined = apiOrCallNode.parent; | ||
|
||
while ( | ||
currentParentNode && | ||
currentParentNode !== functionContextNode.body && | ||
|
@@ -178,29 +157,28 @@ export const scopeUseTask: Rule.RuleModule = { | |
currentScopeForSearch = currentScopeForSearch.upper; | ||
} | ||
|
||
if (!variable) { | ||
// Cannot find variable, assume it's not a shadowed global for safety, | ||
// though this state implies an undeclared variable (another ESLint rule should catch this). | ||
return false; | ||
// If we didn't find a variable, it might be a global API or an undeclared variable. | ||
if (!GLOBALAPIS.includes(identifierNode.name)) { | ||
return true; | ||
} | ||
|
||
if (variable.defs.length === 0) { | ||
if (variable?.defs.length === 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because you removed the !variable test, now you needed to add this. Just add the !varaible test again |
||
// No definitions usually means it's an implicit global (e.g., 'process' in Node.js environment). | ||
// Such a variable is NOT considered "shadowed by a user declaration". | ||
return false; | ||
} | ||
|
||
// If there are definitions, check if any of them are standard declaration types. | ||
// This means the identifier refers to a user-declared variable, parameter, function, class, or an import. | ||
return variable.defs.some((def) => { | ||
return variable?.defs.some((def) => { | ||
return ( | ||
def.type === 'Variable' || | ||
def.type === 'Parameter' || | ||
def.type === 'FunctionName' || | ||
def.type === 'ClassName' || | ||
def.type === 'ImportBinding' | ||
); | ||
}); | ||
}) as boolean; | ||
} | ||
|
||
/** | ||
|
@@ -418,7 +396,6 @@ export const scopeUseTask: Rule.RuleModule = { | |
targetFunctionNode.body.type === AST_NODE_TYPES.BlockStatement | ||
? targetFunctionNode.body | ||
: targetFunctionNode.body; | ||
|
||
analyzeNodeContent(nodeToAnalyze, targetFunctionNode, callNode); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,15 +4,16 @@ | |
// Expect error: { "messageId": "unsafeApiUsageInCalledFunction" } | ||
|
||
import { component$, useTask$ } from '@qwik.dev/core'; | ||
|
||
export default component$(() => { | ||
function child_process() {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is valid code, because it shadows the global child_process, and shouldn't fail the eslint test |
||
useTask$(() => { | ||
function foo() { | ||
process.env; | ||
} | ||
const foo2 = () => { | ||
process.env; | ||
}; | ||
child_process(); | ||
foo(); | ||
foo2(); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,25 @@ | ||
import { component$, isServer, useTask$ } from '@qwik.dev/core'; | ||
|
||
import path from 'path'; | ||
export default component$(() => { | ||
useTask$(() => { | ||
function child_process() {} | ||
function foo() { | ||
if (isServer) { | ||
process.env; | ||
const m = process; | ||
const _path = path; | ||
const pathJoin = path.join('foo', 'bar'); | ||
} | ||
} | ||
child_process(); | ||
const foo2 = () => { | ||
if (isServer) { | ||
process.env; | ||
const m = process; | ||
} | ||
}; | ||
foo(); | ||
foo2(); | ||
}); | ||
return <></>; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { component$, useTask$, isBrowser, useSignal } from '@qwik.dev/core'; | ||
|
||
export default component$(() => { | ||
const state = useSignal(true); | ||
useTask$(({ track }) => { | ||
if (isBrowser) { | ||
track(() => { | ||
if (state.value) { | ||
const values = [ | ||
{ | ||
path: '1', | ||
}, | ||
]; | ||
} | ||
}); | ||
} | ||
}); | ||
return <></>; | ||
}); |
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 seems wrong? It will almost always be true, and if it is correct then it should be befor the while loop because it's not using the result of the while loop