-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: migrate package to TypeScript and publish types #534
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
Changes from all commits
2838607
8582fab
046e58d
d554cd3
bc56d00
7e346f5
98cf8a3
ce6e93b
baedd5b
5cee9cd
cb1d41d
562fa66
9c5e68e
3694a1e
02314b4
d86155c
326cfce
dbac215
912ab48
16430e0
c3b8257
6080c2a
dbab18b
4a187f5
10b1135
80b563c
900ae8c
0eb77cc
11fdcbf
1ce2806
ab52425
b797187
44c1bb4
7404dec
db48780
6f0cd12
cf361fa
833e1d1
0fe29c2
b1ea5c4
02e1d60
6077cd4
c460aff
aa681ad
be5b2bd
96987fa
6035735
9e11e48
1c64770
30a79ee
2b1d318
ba1d108
93dd0ac
ac937db
e97458e
cb5eda9
ff74f46
4bfc4b0
4da3c01
9a6060c
0259d01
2559024
a3cf33a
0052ccf
ead0948
d91a0af
1ffacf7
ff19420
64429ad
3768cfa
2f80268
1abe593
22cf264
88bc975
9f0af6f
fb85ff6
1f22290
6890444
ce4f96f
11fe179
f287566
9eeef96
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 |
---|---|---|
@@ -1,10 +1,11 @@ | ||
.idea | ||
coverage | ||
.idea/ | ||
coverage/ | ||
.vscode | ||
node_modules/ | ||
npm-debug.log | ||
yarn.lock | ||
.eslintcache | ||
dist/ | ||
|
||
# eslint-remote-tester | ||
eslint-remote-tester-results |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,10 @@ | |
* @fileoverview An ESLint plugin for linting ESLint plugins | ||
* @author Teddy Katz | ||
*/ | ||
import { createRequire } from 'node:module'; | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Requirements | ||
// ------------------------------------------------------------------------------ | ||
import type { ESLint, Linter, Rule } from 'eslint'; | ||
|
||
import packageMetadata from '../package.json' with { type: 'json' }; | ||
import consistentOutput from './rules/consistent-output.js'; | ||
import fixerReturn from './rules/fixer-return.js'; | ||
import metaPropertyOrdering from './rules/meta-property-ordering.js'; | ||
|
@@ -41,20 +39,56 @@ import requireMetaType from './rules/require-meta-type.js'; | |
import testCasePropertyOrdering from './rules/test-case-property-ordering.js'; | ||
import testCaseShorthandStrings from './rules/test-case-shorthand-strings.js'; | ||
|
||
const require = createRequire(import.meta.url); | ||
|
||
const packageMetadata = require("../package.json") as { | ||
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. I had issues with this which I fixed in qunitjs/eslint-plugin-qunit#584 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. That doesn't really apply in this case, since this file's relative position to the package.json is the same in the dist folder as it is in the lib folder. 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. Nice! Looks like I should be able to move index.js to fix that in my project: |
||
name: string; | ||
version: string; | ||
}; | ||
|
||
const PLUGIN_NAME = packageMetadata.name.replace(/^eslint-plugin-/, ''); | ||
const CONFIG_NAMES = [ | ||
'all', | ||
'all-type-checked', | ||
'recommended', | ||
'rules', | ||
'tests', | ||
'rules-recommended', | ||
'tests-recommended', | ||
] as const; | ||
type ConfigName = (typeof CONFIG_NAMES)[number]; | ||
|
||
const configFilters = { | ||
all: (rule) => !rule.meta.docs.requiresTypeChecking, | ||
const configFilters: Record<ConfigName, (rule: Rule.RuleModule) => boolean> = { | ||
all: (rule: Rule.RuleModule) => | ||
!( | ||
rule.meta?.docs && | ||
'requiresTypeChecking' in rule.meta.docs && | ||
rule.meta.docs.requiresTypeChecking | ||
), | ||
'all-type-checked': () => true, | ||
recommended: (rule) => rule.meta.docs.recommended, | ||
rules: (rule) => rule.meta.docs.category === 'Rules', | ||
tests: (rule) => rule.meta.docs.category === 'Tests', | ||
'rules-recommended': (rule) => | ||
recommended: (rule: Rule.RuleModule) => !!rule.meta?.docs?.recommended, | ||
rules: (rule: Rule.RuleModule) => rule.meta?.docs?.category === 'Rules', | ||
tests: (rule: Rule.RuleModule) => rule.meta?.docs?.category === 'Tests', | ||
'rules-recommended': (rule: Rule.RuleModule) => | ||
configFilters.recommended(rule) && configFilters.rules(rule), | ||
'tests-recommended': (rule) => | ||
'tests-recommended': (rule: Rule.RuleModule) => | ||
configFilters.recommended(rule) && configFilters.tests(rule), | ||
}; | ||
|
||
const createConfig = (configName: ConfigName): Linter.Config => ({ | ||
name: `${PLUGIN_NAME}/${configName}`, | ||
plugins: { | ||
get [PLUGIN_NAME](): ESLint.Plugin { | ||
return plugin; | ||
}, | ||
}, | ||
rules: Object.fromEntries( | ||
(Object.keys(allRules) as (keyof typeof allRules)[]) | ||
.filter((ruleName) => configFilters[configName](allRules[ruleName])) | ||
.map((ruleName) => [`${PLUGIN_NAME}/${ruleName}`, 'error']), | ||
), | ||
}); | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Plugin Definition | ||
// ------------------------------------------------------------------------------ | ||
|
@@ -93,34 +127,23 @@ const allRules = { | |
'require-meta-type': requireMetaType, | ||
'test-case-property-ordering': testCasePropertyOrdering, | ||
'test-case-shorthand-strings': testCaseShorthandStrings, | ||
}; | ||
} satisfies Record<string, Rule.RuleModule>; | ||
|
||
/** @type {import("eslint").ESLint.Plugin} */ | ||
const plugin = { | ||
meta: { | ||
name: packageMetadata.name, | ||
version: packageMetadata.version, | ||
}, | ||
rules: allRules, | ||
configs: {}, // assigned later | ||
}; | ||
|
||
// configs | ||
Object.assign( | ||
plugin.configs, | ||
Object.keys(configFilters).reduce((configs, configName) => { | ||
return Object.assign(configs, { | ||
[configName]: { | ||
name: `${PLUGIN_NAME}/${configName}`, | ||
plugins: { [PLUGIN_NAME]: plugin }, | ||
rules: Object.fromEntries( | ||
Object.keys(allRules) | ||
.filter((ruleName) => configFilters[configName](allRules[ruleName])) | ||
.map((ruleName) => [`${PLUGIN_NAME}/${ruleName}`, 'error']), | ||
), | ||
}, | ||
}); | ||
}, {}), | ||
); | ||
configs: { | ||
all: createConfig('all'), | ||
'all-type-checked': createConfig('all-type-checked'), | ||
recommended: createConfig('recommended'), | ||
rules: createConfig('rules'), | ||
tests: createConfig('tests'), | ||
'rules-recommended': createConfig('rules-recommended'), | ||
'tests-recommended': createConfig('tests-recommended'), | ||
}, | ||
} satisfies ESLint.Plugin; | ||
|
||
export default plugin; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,21 +2,37 @@ | |
* @fileoverview require fixer functions to return a fix | ||
* @author 薛定谔的猫<[email protected]> | ||
*/ | ||
|
||
import { getStaticValue } from '@eslint-community/eslint-utils'; | ||
import type { Rule } from 'eslint'; | ||
import type { | ||
ArrowFunctionExpression, | ||
FunctionExpression, | ||
Identifier, | ||
Node, | ||
Position, | ||
SourceLocation, | ||
} from 'estree'; | ||
|
||
import { | ||
getContextIdentifiers, | ||
isAutoFixerFunction, | ||
isSuggestionFixerFunction, | ||
} from '../utils.js'; | ||
import type { FunctionInfo } from '../types.js'; | ||
|
||
const DEFAULT_FUNC_INFO: FunctionInfo = { | ||
upper: null, | ||
codePath: null, | ||
hasReturnWithFixer: false, | ||
hasYieldWithFixer: false, | ||
shouldCheck: false, | ||
node: null, | ||
}; | ||
|
||
// ------------------------------------------------------------------------------ | ||
// Rule Definition | ||
// ------------------------------------------------------------------------------ | ||
|
||
/** @type {import('eslint').Rule.RuleModule} */ | ||
const rule = { | ||
const rule: Rule.RuleModule = { | ||
meta: { | ||
type: 'problem', | ||
docs: { | ||
|
@@ -25,36 +41,32 @@ const rule = { | |
recommended: true, | ||
url: 'https://github.com/eslint-community/eslint-plugin-eslint-plugin/tree/HEAD/docs/rules/fixer-return.md', | ||
}, | ||
fixable: null, | ||
fixable: undefined, | ||
schema: [], | ||
messages: { | ||
missingFix: 'Fixer function never returned a fix.', | ||
}, | ||
}, | ||
|
||
create(context) { | ||
let funcInfo = { | ||
upper: null, | ||
codePath: null, | ||
hasReturnWithFixer: false, | ||
hasYieldWithFixer: false, | ||
shouldCheck: false, | ||
node: null, | ||
}; | ||
let contextIdentifiers; | ||
let funcInfo: FunctionInfo = DEFAULT_FUNC_INFO; | ||
let contextIdentifiers = new Set<Identifier>(); | ||
|
||
/** | ||
* As we exit the fix() function, ensure we have returned or yielded a real fix by this point. | ||
* If not, report the function as a violation. | ||
* | ||
* @param {ASTNode} node - A node to check. | ||
* @param {Location} loc - Optional location to report violation on. | ||
* @returns {void} | ||
* @param node - A node to check. | ||
* @param loc - Optional location to report violation on. | ||
*/ | ||
function ensureFunctionReturnedFix( | ||
node, | ||
loc = (node.id || node).loc.start, | ||
) { | ||
node: ArrowFunctionExpression | FunctionExpression, | ||
loc: Position | SourceLocation | undefined = (node.type === | ||
'FunctionExpression' && node.id | ||
? node.id | ||
: node | ||
).loc?.start, | ||
): void { | ||
if ( | ||
(node.generator && !funcInfo.hasYieldWithFixer) || // Generator function never yielded a fix | ||
(!node.generator && !funcInfo.hasReturnWithFixer) // Non-generator function never returned a fix | ||
|
@@ -70,10 +82,9 @@ const rule = { | |
/** | ||
* Check if a returned/yielded node is likely to be a fix or not. | ||
* A fix is an object created by fixer.replaceText() for example and returned by the fix function. | ||
* @param {ASTNode} node - node to check | ||
* @returns {boolean} | ||
* @param node - node to check | ||
michaelfaith marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
function isFix(node) { | ||
function isFix(node: Node): boolean { | ||
if (node.type === 'ArrayExpression' && node.elements.length === 0) { | ||
// An empty array is not a fix. | ||
return false; | ||
|
@@ -104,22 +115,22 @@ const rule = { | |
}, | ||
|
||
// Stacks this function's information. | ||
onCodePathStart(codePath, node) { | ||
onCodePathStart(codePath: Rule.CodePath, node: Node) { | ||
funcInfo = { | ||
upper: funcInfo, | ||
codePath, | ||
hasYieldWithFixer: false, | ||
hasReturnWithFixer: false, | ||
shouldCheck: | ||
isAutoFixerFunction(node, contextIdentifiers) || | ||
isSuggestionFixerFunction(node, contextIdentifiers), | ||
isAutoFixerFunction(node, contextIdentifiers, context) || | ||
isSuggestionFixerFunction(node, contextIdentifiers, context), | ||
node, | ||
}; | ||
}, | ||
|
||
// Pops this function's information. | ||
onCodePathEnd() { | ||
funcInfo = funcInfo.upper; | ||
funcInfo = funcInfo.upper ?? DEFAULT_FUNC_INFO; | ||
}, | ||
|
||
// Yield in generators | ||
|
@@ -147,7 +158,7 @@ const rule = { | |
'ArrowFunctionExpression:exit'(node) { | ||
if (funcInfo.shouldCheck) { | ||
const sourceCode = context.sourceCode; | ||
const loc = sourceCode.getTokenBefore(node.body).loc; // Show violation on arrow (=>). | ||
const loc = sourceCode.getTokenBefore(node.body)?.loc; // Show violation on arrow (=>). | ||
if (node.expression) { | ||
// When the return is implied (no curly braces around the body), we have to check the single body node directly. | ||
if (!isFix(node.body)) { | ||
|
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.
Technically, I believe we should be using
npm ci
in these workflows. Fine to consider it as a follow-up.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.
Yeah, I just matched the other steps. Could be a follow-up that changes them all to ci?
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.
I was thinking a bit more on this later. In order to use
npm ci
, it'll require than just updating the calls in the workflow. The repo would need to start maintaining a lock file. It currently has lock files explicitly disabled, andnpm ci
requires that. I personally don't think having a lock file is a bad thing. I was kind of surprised when I first realized there wasn't one. But it seemed to be a conscious choice someone made at some point.The other consideration is that using
npm ci
in the builds will result in longer build times, if that's important to you.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.
Oh good catch. I now remember that's why we don't use
npm ci
. Let's avoid adding it at this time. For us, it's fine to be more aggressive with dependency updates and omit a lockfile.