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

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

Open
runspired opened this issue Dec 15, 2022 · 3 comments · May be fixed by #1700
Open

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

runspired opened this issue Dec 15, 2022 · 3 comments · May be fixed by #1700
Labels
enhancement New Rule Idea for a new lint rule

Comments

@runspired
Copy link

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
Copy link
Contributor

ef4 commented Dec 15, 2022

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.

@runspired runspired linked a pull request Dec 15, 2022 that will close this issue
@Windvis
Copy link
Contributor

Windvis commented Jan 26, 2023

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
Copy link

Is it desirable though?

ember-codemods/ember-native-class-codemod#495 (comment) suggests decorators before export is valid now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New Rule Idea for a new lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants