-
-
Notifications
You must be signed in to change notification settings - Fork 212
template-missing-invokable: add built-in invokables and always auto-fix from config #2556
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 3 commits
e2747b4
c11b89a
1985afa
3eec111
cf2a5cd
68963ce
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,3 +1,58 @@ | ||
| 'use strict'; | ||
|
|
||
| const path = require('node:path'); | ||
| const fs = require('node:fs'); | ||
|
|
||
| // Packages that ship with Ember/Glimmer are always available to auto-fix. | ||
| function isBuiltinPackage(moduleName) { | ||
| return moduleName.startsWith('@ember/') || moduleName.startsWith('@glimmer/'); | ||
| } | ||
|
|
||
| // Returns the root package name from a module specifier, e.g. | ||
| // 'ember-truth-helpers' -> 'ember-truth-helpers' | ||
| // 'ember-truth-helpers/helpers' -> 'ember-truth-helpers' | ||
| // '@scope/pkg/deep' -> '@scope/pkg' | ||
| function rootPackageName(moduleName) { | ||
| if (moduleName.startsWith('@')) { | ||
| const parts = moduleName.split('/'); | ||
| return parts.slice(0, 2).join('/'); | ||
| } | ||
| return moduleName.split('/')[0]; | ||
| } | ||
|
|
||
| // Walk up the directory tree from startDir to find the nearest package.json. | ||
| function findNearestPackageJson(startDir) { | ||
| let dir = startDir; | ||
| let parent = path.dirname(dir); | ||
| while (dir !== parent) { | ||
| const candidate = path.join(dir, 'package.json'); | ||
| if (fs.existsSync(candidate)) { | ||
| return candidate; | ||
| } | ||
| dir = parent; | ||
| parent = path.dirname(dir); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| function isPackageInProjectDeps(moduleName, fileDir) { | ||
|
Contributor
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. @copilot we need a cache when doing this -- we can't be hitting the FS every checkInvokable
Contributor
Author
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. Added in commit 3eec111. |
||
| try { | ||
| const pkgPath = findNearestPackageJson(fileDir); | ||
| if (!pkgPath) { | ||
| return false; | ||
| } | ||
| const packageJson = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); | ||
| const pkg = rootPackageName(moduleName); | ||
| return Boolean( | ||
| (packageJson.dependencies && pkg in packageJson.dependencies) || | ||
| (packageJson.devDependencies && pkg in packageJson.devDependencies) || | ||
| (packageJson.peerDependencies && pkg in packageJson.peerDependencies) | ||
| ); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** @type {import('eslint').Rule.RuleModule} */ | ||
| module.exports = { | ||
| meta: { | ||
|
|
@@ -41,14 +96,23 @@ module.exports = { | |
| if (!isBound(node.path.head, sourceCode.getScope(node.path))) { | ||
| const matched = context.options[0]?.invokables?.[node.path.head.name]; | ||
| if (matched) { | ||
| const [name, module] = matched; | ||
| const importStatement = buildImportStatement(node.path.head.name, name, module); | ||
| const [name, moduleName] = matched; | ||
| const fileDir = path.dirname( | ||
| path.resolve(context.getPhysicalFilename?.() ?? context.getFilename()) | ||
| ); | ||
| const canAutoFix = | ||
| isBuiltinPackage(moduleName) || | ||
| isPackageInProjectDeps(moduleName, fileDir); | ||
|
|
||
| const importStatement = buildImportStatement(node.path.head.name, name, moduleName); | ||
| context.report({ | ||
| node: node.path, | ||
| messageId: 'missing-invokable', | ||
| fix(fixer) { | ||
| return fixer.insertTextBeforeRange([0, 0], `${importStatement};\n`); | ||
| }, | ||
| fix: canAutoFix | ||
| ? function (fixer) { | ||
| return fixer.insertTextBeforeRange([0, 0], `${importStatement};\n`); | ||
| } | ||
| : null, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "name": "has-ember-truth-helpers", | ||
| "dependencies": { | ||
| "ember-truth-helpers": "*" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,20 @@ | |
| // Requirements | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| const path = require('node:path'); | ||
| const rule = require('../../../lib/rules/template-missing-invokable'); | ||
| const RuleTester = require('eslint').RuleTester; | ||
|
|
||
| //------------------------------------------------------------------------------ | ||
| // Helpers | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| // A filename inside a fixture project that has ember-truth-helpers installed. | ||
| const filenameInProjectWithTruthHelpers = path.join( | ||
| __dirname, | ||
| '../../fixtures/projects/has-ember-truth-helpers/test.gjs' | ||
| ); | ||
|
|
||
| //------------------------------------------------------------------------------ | ||
| // Tests | ||
| //------------------------------------------------------------------------------ | ||
|
|
@@ -68,8 +79,30 @@ ruleTester.run('template-missing-invokable', rule, { | |
| ], | ||
|
|
||
| invalid: [ | ||
| // Subexpression invocations | ||
| // Subexpression invocations — no auto-fix when package is not in project deps | ||
| { | ||
| code: ` | ||
| <template> | ||
| {{#if (eq 1 1)}} | ||
| They're equal | ||
| {{/if}} | ||
| </template> | ||
| `, | ||
| output: null, | ||
| options: [ | ||
| { | ||
| invokables: { | ||
| eq: ['eq', 'ember-truth-helpers'], | ||
| }, | ||
| }, | ||
| ], | ||
|
|
||
| errors: [{ type: 'GlimmerPathExpression', message: rule.meta.messages['missing-invokable'] }], | ||
| }, | ||
|
|
||
| // Subexpression invocations — auto-fix when package IS in project deps | ||
| { | ||
| filename: filenameInProjectWithTruthHelpers, | ||
| code: ` | ||
| <template> | ||
| {{#if (eq 1 1)}} | ||
|
|
@@ -96,19 +129,14 @@ ruleTester.run('template-missing-invokable', rule, { | |
| errors: [{ type: 'GlimmerPathExpression', message: rule.meta.messages['missing-invokable'] }], | ||
| }, | ||
|
|
||
| // Mustache Invocations | ||
|
Contributor
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. @copilot why'd you remove these?
Contributor
Author
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. When I dropped the |
||
| // Mustache Invocations — no auto-fix when package is not in project deps | ||
| { | ||
| code: ` | ||
| <template> | ||
| {{eq 1 1}} | ||
| </template> | ||
| `, | ||
| output: `import { eq } from 'ember-truth-helpers'; | ||
|
|
||
| <template> | ||
| {{eq 1 1}} | ||
| </template> | ||
| `, | ||
| output: null, | ||
| options: [ | ||
| { | ||
| invokables: { | ||
|
|
@@ -119,6 +147,7 @@ ruleTester.run('template-missing-invokable', rule, { | |
| errors: [{ type: 'GlimmerPathExpression', message: rule.meta.messages['missing-invokable'] }], | ||
| }, | ||
| { | ||
| filename: filenameInProjectWithTruthHelpers, | ||
| code: ` | ||
| import MyComponent from 'somewhere'; | ||
| <template> | ||
|
|
@@ -142,7 +171,7 @@ ruleTester.run('template-missing-invokable', rule, { | |
| errors: [{ type: 'GlimmerPathExpression', message: rule.meta.messages['missing-invokable'] }], | ||
| }, | ||
|
|
||
| // Modifier Inovcations | ||
| // Modifier Invocations — built-in package always auto-fixes | ||
| { | ||
| code: ` | ||
| function doSomething() {} | ||
|
|
@@ -167,8 +196,9 @@ ruleTester.run('template-missing-invokable', rule, { | |
| errors: [{ type: 'GlimmerPathExpression', message: rule.meta.messages['missing-invokable'] }], | ||
| }, | ||
|
|
||
| // Multiple copies of a fixable invocation | ||
| // Multiple copies of a fixable invocation — with package installed | ||
| { | ||
| filename: filenameInProjectWithTruthHelpers, | ||
| code: ` | ||
| let other = <template> | ||
| {{#if (eq 3 3) }} | ||
|
|
@@ -216,8 +246,9 @@ ruleTester.run('template-missing-invokable', rule, { | |
| ], | ||
| }, | ||
|
|
||
| // Auto-fix with a default export | ||
| // Auto-fix with a default export — package installed | ||
| { | ||
| filename: filenameInProjectWithTruthHelpers, | ||
| code: ` | ||
| <template> | ||
| {{#if (eq 1 1)}} | ||
|
|
||
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 let's git rid of the package.json reading actually -- it needs to be done very carefully, and I don't want to walk you through that right now. lets just rely on the config for the lint rule.
Also, we need a list of default importables from the template:
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.
Done in cf2a5cd. All FS/package.json reading is removed —
path,fs, the cache, and all related helpers are gone. Auto-fix now runs unconditionally whenever an invokable is matched.Added
BUILTIN_INVOKABLESwith the defaults you listed:User-provided
invokablesconfig is spread on top, so any built-in can be overridden.