ember-cli / eslint-plugin-ember

An ESLint plugin that provides set of rules for Ember applications based on commonly known good practices.
MIT License
260 stars 202 forks source link

New Rule: no-decorators-before-export #1699

Open runspired opened 1 year ago

runspired commented 1 year ago

In the very first TC39 proposal for decorators, a syntax option was considered to allow the following:

import classic from 'ember-classic-decorators';
import Component from '@ember/component';

@classic
export default class MyComponent extends Component {}

However, in the various specs that followed, including in the current proposal that is stage-3, this syntax was explicitly disallowed. The problem it causes is a little more apparent when put on one line:

@classic export default class MyComponent extends Component {}

The correct way to specify decorators is onto the class itself prior to exporting the class.

import classic from 'ember-classic-decorators';
import Component from '@ember/component';

@classic
class MyComponent extends Component {}

export default MyComponent;

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

This causes a lot of issues:

1) being non-spec, non-babel parsers do not support it. This means codemods using jscodeshift, rollup using acorn, and any number of other tools or code compilers error when they encounter these files.

2) its easy for transpilation to go very wrong. In the case of the @classic decorator for instance, it will not properly apply as the transform will skip the insertion of class into the map of classic classes: thereby making its utility for ensuring sub-classes conform to the same rules void.

3) it can lead to compounding issues during transpilation when the class is left unnamed.

The following is valid JS:

import Component from '@ember/component';

export default class extends Component {}

However, when using a class decorator this can be problematic

import { tagName } from '@ember-decorators/component';
import Component from '@ember/component';

@tagName('ul')
export default class extends Component {}

As the lack of a named class to apply to leads this to be effectively transpiled to the following (no decorator applied)

const _default = class extends Component {}
exports.default = _default;

While it is true these are both bugs that should probably get fixed somewhere upstream (or via a custom transform since the babel legacy transform is no longer maintained), considering that this syntax was non-spec even at the point that ember adopted decorators, and considering that the correct approach works as expected, we should lint against the non-standard approach.

ef4 commented 1 year ago

This seems good to me. I can't see any downside to linting against a spec-violating syntax, even if the implementation happens to still allow it for now.

Windvis commented 1 year ago

While I like the idea of this rule, I don't think this issue is limited to the Ember ecosystem alone? Would it make sense to try to upstream it to eslint instead?

tehhowch commented 1 year ago

Is it desirable though?

https://github.com/ember-codemods/ember-native-class-codemod/issues/495#issuecomment-1450707075 suggests decorators before export is valid now.