🐛 This variable is unused when import decorator

[harrytran998]

Environment information

System:
    OS: macOS 12.3.1
  Binaries:
    Node: 18.6.0 - /usr/local/bin/node
    Yarn: 3.2.2 - /usr/local/bin/yarn
    npm: 8.13.2 - /usr/local/bin/npm
  Browsers:
    Chrome: 105.0.5195.102
    Firefox: 100.0
    Safari: 15.4

---

TS CONFIG file

{
  "compilerOptions": {
    "outDir": "./dist",
    "baseUrl": "./",
    "module": "commonjs",
    "declaration": true,
    "removeComments": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "allowSyntheticDefaultImports": true,
    "target": "es2017",
    "sourceMap": true,
    "incremental": true,
    "skipLibCheck": true,
    "strictPropertyInitialization": false
  },
  "exclude": ["node_modules", "dist"]
}

What happened?

Our project uses NestJs for backend API. So we must use the decorator feature.

But currently, seem Rome does not detect this correctly.

import { Controller, Get } from '@nestjs/common';

@Controller()
export class WelcomeController {
    @Get()
    welcome() {
        return {
            message: 'Welcome to OneMind API',
        };
    }
}

Here the code playground

https://play.rome.tools/?code=import+%7B+Controller%2C+Get+%7D+from+%27%40nestjs%2Fcommon%27%3B%0A%0A%40Controller%28%29%0Aexport+class+WelcomeController+%7B%0A%09%40Get%28%29%0A%09welcome%28%29+%7B%0A%09%09return+%7B%0A%09%09%09message%3A+%27Welcome+to+OneMind+API%27%2C%0A%09%09%7D%3B%0A%09%7D%0A%7D%0A&lineWidth=80&indentStyle=tab&quoteStyle=double&quoteProperties=as-needed&indentWidth=2&sourceType=module&typescript=true&jsx=false#aQBtAHAAbwByAHQAIAB7ACAAQwBvAG4AdAByAG8AbABsAGUAcgAsACAARwBlAHQAIAB9ACAAZgByAG8AbQAgACcAQABuAGUAcwB0AGoAcwAvAGMAbwBtAG0AbwBuACcAOwAKAAoAQABDAG8AbgB0AHIAbwBsAGwAZQByACgAKQAKAGUAeABwAG8AcgB0ACAAYwBsAGEAcwBzACAAVwBlAGwAYwBvAG0AZQBDAG8AbgB0AHIAbwBsAGwAZQByACAAewAKAAkAQABHAGUAdAAoACkACgAJAHcAZQBsAGMAbwBtAGUAKAApACAAewAKAAkACQByAGUAdAB1AHIAbgAgAHsACgAJAAkACQBtAGUAcwBzAGEAZwBlADoAIAAnAFcAZQBsAGMAbwBtAGUAIAB0AG8AIABPAG4AZQBNAGkAbgBkACAAQQBQAEkAJwAsAAoACQAJAH0AOwAKAAkAfQAKAH0ACgA=

Expected result

It works correctly like import type or something like that 😆

Code of Conduct

• [X] I agree to follow Rome's Code of Conduct

[IWANABETHATGUY]

This is the limitation of our parser, it would skip parsing decorator (treat it as trivia), we will probably implement this syntax after Typescript landing it(the decorator in typescript is not the same as ECMA spec). The decorator has been add to their 4.9 iteration plan

[harrytran998]

Thank you for the quick response - according to my understanding, currently, we should temporarily disable this rule until the 4.9 iteration plan.

Bcs we use monorepo on our project if we disable this rule --> Affects all packages. But seem Rome now doesn't support config for each folder(`overwrite), file like Eslint, Prettier. So do you have any suggestions for that @IWANABETHATGUY ?

[IWANABETHATGUY]

@ematipico , any thoughts?

[ematipico]

That's a good question, and I am not sure I have the right answer.

Theoretically, we might be able to fix the rule by looking at the Skipped trivia, which where the decorators are stored, although I am not sure it's worth the effort. Decorators are now at stage 3, which means that soon we will have to start working on their support.

On the other hand, this proposal is actually different from the previous one, which we don't plan to support (the previous one). There's also the fact that this proposal has been stripped out of the Reflect.metadata, which is at the core of many frameworks such as Nest JS and typeorm, and I don't see these frameworks to use the proposal any time soon.

[leops]

I think that even if our grammar currently doesn't support decorators, these skipped tokens are still getting parsed as identifiers, and since the semantic model cannot precisely interpret what's going on there, we could conservatively emit fake read and write events for all the identifiers we find in skipped tokens sections. @xunilrj would this work ?

[MichaReiser]

I think that even if our grammar currently doesn't support decorators, these skipped tokens are still getting parsed as identifiers, and since the semantic model cannot precisely interpret what's going on there, we could conservatively emit fake read and write events for all the identifiers we find in skipped tokens sections. @xunilrj would this work ?

How would you envision that? Our parser only returns skipped token trivia. It's no longer know if any of those used to be an identifier before.

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity.

[xunilrj]

The problem is that the "@Controller" today is parsed as

0: EXPORT_KW@49..72 "export" [Newline("\n"), Newline("\n"), Skipped("@"), Skipped("Controller"), Skipped("("), Skipped(")"), Newline("\n")] [Whitespace(" ")]

The only for the semantic model to generate reference events from this would be to parse it. For this probably makes more sense to parse this inside the parser itself.

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity.

[jpike88]

the decorator in typescript is not the same as ECMA spec

What popular JS-only frameworks is using decorators according to the ECMA spec? Typescript is immensely popular nowadays, why not consider them the defacto standard when it comes to decorators (considering Angular relies heavily on decorators), and merge to ECMA when it makes sense to do so? Surely it doesn't differ by that much.

[jpike88]

Just read more on the TS side, looks like the new decorators won't land until TS 5.0 https://github.com/microsoft/TypeScript/issues/51362

[jpike88]

https://github.com/tc39/proposal-decorators/issues/47#issuecomment-1094077700

https://github.com/microsoft/vscode/blob/33040b9696aa918784c818c5d375ec4a4c1f8e85/src/vs/code/electron-main/auth.ts#L67

This is where there's a good argument for proceeding with how decorators are defined right now.

Parameter decorators are heavily used in frameworks like Angular, and also heavily used in projects like the VSCode source code.

The currently Stage 3 proposal does not mention parameter decorators. This is unlikely to change/resolve for another couple of years or so, if it does at all, which kind of makes the ECMA decorator standard a ghost standard until it it starts to cover popular projects out there. There's a big disconnect between what ECMA defines and reality, and that isn't going to change anytime soon.

[elijaholmos]

I am also experiencing this issue in a Nest.js project. A possible workaround is for Rome to "overlook" decorators or even imported variables and only lint noUnusedVariables that are defined in the scope of each individual file.

[github-actions[bot]]

👋 @rome/staff please triage this issue by adding one of the following labels: S-Bug: confirmed, S-Planned , S-Wishlist or umbrella

[jpike88]

as mentioned in #4049, I think there should be a stopgap solution that allows such an important rule to be able to be used. The bare minimum being parsing @decorator values just so the import statements can be considered 'used'. This rule is so important and it would be extremely disadvantageous for an angular developer to use Rome without that rule in place.

[harrytran998]

Yup! I am still here waiting for the Rome team to upgrade this 😆. Not only Angular FE dev, this includes more like Lite, NestJs, Knex, Sequelize... all the tools use a decorator feature 😆!

[jpike88]

looks like this can be closed due to #4252

[ematipico]

Not yet, the feature hasn't been released. Let's close when the feature is released (at least in a nightly)

[harrytran998]

Haha, near 8 months to complete this 🤣 - Thanks for your team's work hard 🥰 We really appreciate this.

[ematipico]

The Stage 3 decorators have been implemented and the code works now. Closing!

FYI these are the new Stage 3 decorators, so decorator parameters won't work just yet.

This means that this code:

class B {
    get(@Get params) {}
}

Will still throw errors.