Skip to content
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

Allow generic type in TypeScript class in no-empty-glimmer-component-classes rule #1876

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion docs/rules/no-empty-glimmer-component-classes.md
Expand Up @@ -8,7 +8,7 @@ This rule will catch and prevent the use of empty backing classes for Glimmer co

## Rule Details

This rule aims to disallow the use of empty backing classes for Glimmer components when possible including only using ember template tags in your Glimmer component. Template-only Glimmer components where there is no backing class are much faster and lighter-weight than Glimmer components with backing classes, which are much lighter-weight than Ember components. Therefore, you should only have a backing class for a Glimmer component when absolutely necessary.
This rule aims to disallow the use of empty backing classes for Glimmer components when possible including only using ember template tags in your Glimmer component. Template-only Glimmer components where there is no backing class are much faster and lighter-weight than Glimmer components with backing classes, which are much lighter-weight than Ember components. Therefore, you should only have a backing class for a Glimmer component when absolutely necessary. An exception to this case is if you need the component to be generic over part of its type signature.

To fix violations of this rule:

Expand All @@ -34,6 +34,14 @@ export default class MyComponent extends Component {
}
```

```ts
import Component from '@glimmer/component';

export interface TypeSig {}

export default class MyComponent extends Component<TypeSig> {}
```

Examples of **correct** code for this rule:

```js
Expand Down Expand Up @@ -74,6 +82,15 @@ export default class MyComponent extends Component {
}
```

```ts
import Component from '@glimmer/component';

export interface SomeSig {}
export interface SomeOtherSig {}

export default class MyComponent<SomeSig> extends Component<SomeOtherSig> {}
```

## References

- [Glimmer Components - Octane Upgrade Guide](https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/)
Expand Down
12 changes: 8 additions & 4 deletions lib/rules/no-empty-glimmer-component-classes.js
Expand Up @@ -31,14 +31,18 @@ module.exports = {
create(context) {
return {
ClassDeclaration(node) {
const nodeIsGlimmerComponent = isGlimmerComponent(context, node);
if (nodeIsGlimmerComponent && !node.decorators && node.body.body.length === 0) {
if (!isGlimmerComponent(context, node)) {
return;
}

const subClassHasTypeDefinition = node.typeParameters?.params?.length > 0;
if (!node.decorators && node.body.body.length === 0 && !subClassHasTypeDefinition) {
context.report({ node, message: ERROR_MESSAGE });
} else if (
nodeIsGlimmerComponent &&
node.body.body.length === 1 &&
isClassPropertyOrPropertyDefinition(node.body.body[0]) &&
node.body.body[0].key?.callee?.name === '__GLIMMER_TEMPLATE'
node.body.body[0].key?.callee?.name === '__GLIMMER_TEMPLATE' &&
!subClassHasTypeDefinition
) {
context.report({ node, message: ERROR_MESSAGE_TEMPLATE_TAG });
}
Expand Down
36 changes: 27 additions & 9 deletions tests/lib/rules-preprocessor/gjs-gts-processor-test.js
Expand Up @@ -13,10 +13,10 @@ const plugin = require('../../../lib');
/**
* Helper function which creates ESLint instance with enabled/disabled autofix feature.
*
* @param {CLIEngineOptions} [options={}] Whether to enable autofix feature.
* @param {String} parser The parser to use.
* @returns {ESLint} ESLint instance to execute in tests.
*/
function initESLint(options) {
function initESLint(parser = '@babel/eslint-parser') {
// tests must be run with ESLint 7+
return new ESLint({
ignore: false,
Expand All @@ -27,7 +27,7 @@ function initESLint(options) {
env: {
browser: true,
},
parser: '@babel/eslint-parser',
parser,
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
Expand All @@ -45,7 +45,6 @@ function initESLint(options) {
'ember/no-array-prototype-extensions': 'error',
},
},
...options,
});
}

Expand Down Expand Up @@ -99,6 +98,24 @@ const valid = [
}
`,
},
{
filename: 'my-component.gts',
code: `import Component from '@glimmer/component';

interface ListSignature<T> {
Args: {
items: Array<T>;
};
Blocks: {
default: [item: T]
};
}

export default class List<T> extends Component<ListSignature<T>> {
<template>Hello!</template>
}`,
parser: '@typescript-eslint/parser',
},
/**
* TODO: SKIP this scenario. Tracked in https://github.com/ember-cli/eslint-plugin-ember/issues/1685
{
Expand All @@ -118,7 +135,7 @@ const invalid = [
{
filename: 'my-component.gjs',
code: `import Component from '@glimmer/component';
export default class Chris extends Component {
export default class MyComponent extends Component {
<template>Hello!</template>
}`,
errors: [
Expand All @@ -130,6 +147,7 @@ const invalid = [
},
],
},

{
filename: 'my-component.gjs',
code: `
Expand Down Expand Up @@ -272,11 +290,11 @@ const invalid = [
describe('template-vars', () => {
describe('valid', () => {
for (const scenario of valid) {
const { code, filename } = scenario;
const { code, filename, parser } = scenario;

// eslint-disable-next-line jest/valid-title
it(code, async () => {
const eslint = initESLint();
const eslint = initESLint(parser);
const results = await eslint.lintText(code, { filePath: filename });
const resultErrors = results.flatMap((result) => result.messages);

Expand All @@ -296,11 +314,11 @@ describe('template-vars', () => {

describe('invalid', () => {
for (const scenario of invalid) {
const { code, filename, errors } = scenario;
const { code, filename, errors, parser } = scenario;

// eslint-disable-next-line jest/valid-title
it(code, async () => {
const eslint = initESLint();
const eslint = initESLint(parser);
const results = await eslint.lintText(code, { filePath: filename });

const resultErrors = results.flatMap((result) => result.messages);
Expand Down
40 changes: 40 additions & 0 deletions tests/lib/rules/no-empty-glimmer-component-classes.js
Expand Up @@ -34,6 +34,34 @@ ruleTester.run('no-empty-glimmer-component-classes', rule, {

@MyDecorator
class MyComponent extends Component {}`,
{
code: `
import Component from '@glimmer/component';

interface ListSignature<T> {
Args: {
items: Array<T>;
};
Blocks: {
default: [item: T]
};
}

export default class List<T> extends Component<ListSignature<T>> {}
`,
parser: require.resolve('@typescript-eslint/parser'),
},
{
code: `
import Component from '@glimmer/component';

export interface SomeSig {}
export interface SomeOtherSig {}

export default class MyComponent<SomeSig> extends Component<SomeOtherSig> {}
`,
parser: require.resolve('@typescript-eslint/parser'),
},
],
invalid: [
{
Expand All @@ -50,5 +78,17 @@ ruleTester.run('no-empty-glimmer-component-classes', rule, {
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'ClassDeclaration' }],
},
{
code: `
import Component from '@glimmer/component';

export interface TypeSig {}

export default class MyComponent extends Component<TypeSig> {}
`,
output: null,
parser: require.resolve('@typescript-eslint/parser'),
errors: [{ message: ERROR_MESSAGE, type: 'ClassDeclaration' }],
},
],
});