Add new rule `no-decorators-before-export` by runspired · Pull Request #1700 · ember-cli/eslint-plugin-ember

runspired · 2022-12-15T23:38:01Z

resolves #1699

lin-ll

Overall this looks really good. Thank you for both raising this issue and creating a PR for this rule so quickly.

lin-ll · 2022-12-16T01:36:09Z

docs/rules/no-decorators-before-export.md

+
+💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/ember-cli/eslint-plugin-ember#-configurations).
+
+🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).


Maybe worth adding a note that the fixer only works for named exports

lin-ll · 2022-12-16T01:36:40Z

docs/rules/no-decorators-before-export.md

+export default MyComponent;
+```
+
+Unfortunately, in order to maximize compatibility with existing decorators, the second rendition of the spec - the rendition of the spec which Ember adopted (there have been four) - allowed this syntax to still be used if using the babel plugin AND also explicitly allowing it in the plugin config. Ember chose to be maximally capatible and enabled this option.


Suggested change

Unfortunately, in order to maximize compatibility with existing decorators, the second rendition of the spec - the rendition of the spec which Ember adopted (there have been four) - allowed this syntax to still be used if using the babel plugin AND also explicitly allowing it in the plugin config. Ember chose to be maximally capatible and enabled this option.

Unfortunately, in order to maximize compatibility with existing decorators, the second rendition of the spec - the rendition of the spec which Ember adopted (there have been four) - allowed this syntax to still be used if using the babel plugin AND also explicitly allowing it in the plugin config. Ember chose to be maximally compatible and enabled this option.

lin-ll · 2022-12-16T01:37:54Z

docs/rules/no-decorators-before-export.md

+
+@classic
+export class MyKlass extends EmberObject {}
+```


If possible, do you mind adding a couple link references as well, like the proposal for decorators and issues that were caused by this?

bmish · 2022-12-16T02:27:10Z

lib/rules/no-decorators-before-export.js

+      description:
+        'disallow the use of a class decorator on an export declaration. Enforces use of the decorator on the class directly before exporting the result.',
+      category: 'Miscellaneous',
+      recommended: true,


Note: recommended rules are only added in major versions so this should be removed. We can enable it in the next major version.

bmish

Thanks for starting this! One tip is to try running it on a real codebase to ensure there are no missed edge cases / crashes.

bmish · 2022-12-16T03:41:11Z

lib/rules/no-decorators-before-export.js

@@ -0,0 +1,73 @@
+'use strict';
+
+/** @type {import('eslint').Rule.RuleModule} */


This type annotation is supposed to go directly above module.exports = { ....

bmish · 2022-12-16T03:44:13Z

tests/lib/rules/no-decorators-before-export.js

+  invalid: [
+    // basic
+    {
+      code: "import classic from 'ember-classic-decorators';\n@classic\nexport class Foo {}",


I'm finding these single-line strings with \n a hard to read. Normally, we use multi-line template strings.

bmish · 2022-12-16T03:48:48Z

lib/rules/no-decorators-before-export.js

+      const sourceCode = context.getSourceCode();
+      const src = sourceCode.getText(node);


These variables can move inside the fixer since they aren't used outside.

bmish · 2022-12-16T03:49:54Z

lib/rules/no-decorators-before-export.js

+  return (
+    node.declaration &&
+    node.declaration.type === 'ClassDeclaration' &&
+    node.declaration.decorators?.length


Normally we should have a comparison length > 0. Or are you trying to check if the array exists at all? Then Array.isArray() is more clear.

it might not exist, or it may exist and be empty, in either case that is fine.

Okay, well !Array.isArray(node.declaration.decorators) || node.declaration.decorators.length > 0 would convey that intent more clearly to me. Feel free to leave as is if you prefer.

bmish · 2022-12-16T03:50:30Z

lib/rules/no-decorators-before-export.js

+
+/** @type {import('eslint').Rule.RuleModule} */
+
+function reportInvalidSyntax(context, node) {


Can you add a comment explaining the general strategy/approach/what's going on?

bmish · 2022-12-16T03:51:25Z

lib/rules/no-decorators-before-export.js

+  }
+}
+
+function isInvalidSyntax(node) {


Seems like this could just be a generic classDeclarationHasDecorators(node) function. That way, it's more clear what it's doing, and could be reusable in other places.

It isn’t though, it’s checking against an export declaration specifically

My bad, then exportDeclarationHasDecorators?. Feel free to leave it if you want, but I think my own confusion illustrates the point. I'm just thinking of ways to make it more clear about what it's doing and reusable.

runspired · 2022-12-16T04:01:04Z

Thanks for starting this! One tip is to try running it on a real codebase to ensure there are no missed edge cases / crashes.

@bmish it is already running on one

bmish · 2023-01-06T22:51:50Z

@runspired would love to get this rule in still! Are you planning to work on it?

runspired added 3 commits December 15, 2022 15:37

feat: lint no-decorators-before-export

e20d61c

fix lint and tests

4193a63

fix docs

99a1697

lin-ll changed the title ~~feat: lint no-decorators-before-export~~ Add new rule no-decorators-before-export Dec 16, 2022

lin-ll added the enhancement label Dec 16, 2022

lin-ll reviewed Dec 16, 2022

View reviewed changes

bmish reviewed Dec 16, 2022

View reviewed changes

bmish requested changes Dec 16, 2022

View reviewed changes


		💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/ember-cli/eslint-plugin-ember#-configurations).

		🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

	Unfortunately, in order to maximize compatibility with existing decorators, the second rendition of the spec - the rendition of the spec which Ember adopted (there have been four) - allowed this syntax to still be used if using the babel plugin AND also explicitly allowing it in the plugin config. Ember chose to be maximally capatible and enabled this option.
	Unfortunately, in order to maximize compatibility with existing decorators, the second rendition of the spec - the rendition of the spec which Ember adopted (there have been four) - allowed this syntax to still be used if using the babel plugin AND also explicitly allowing it in the plugin config. Ember chose to be maximally compatible and enabled this option.

		@@ -0,0 +1,73 @@
		'use strict';

		/** @type {import('eslint').Rule.RuleModule} */

		const sourceCode = context.getSourceCode();
		const src = sourceCode.getText(node);


		/** @type {import('eslint').Rule.RuleModule} */

		function reportInvalidSyntax(context, node) {

Uh oh!

Conversation

runspired commented Dec 15, 2022

Uh oh!

lin-ll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmish Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmish Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runspired commented Dec 16, 2022

Uh oh!

bmish commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmish Dec 16, 2022 •

edited

Loading

bmish Dec 16, 2022 •

edited

Loading

bmish commented Jan 6, 2023 •

edited

Loading