darraghoriordan / eslint-plugin-nestjs-typed

Some eslint rules for working with NestJs projects
http://www.darraghoriordan.com
171 stars 34 forks source link

injectable-should-be-provided works only with PropertyAssignment #29

Closed WiseBird closed 2 years ago

WiseBird commented 2 years ago

Description

injectable-should-be-provided expect providers to be an array, otherwise it may fail with NPE:

TypeError: Cannot read properties of undefined (reading 'map')

Reproduction

https://github.com/WiseBird/eslint-plugin-nestjs-issue-shorthand (commit)

Details

Works:

@Module({
  imports: [],
  controllers: [AppController],
  providers: [AppService],
})

Moving the array into a variable doesn't:

let providers = [AppService];

@Module({
  imports: [],
  controllers: [AppController],
  providers: providers, <--
})

Shorthand syntax doesn't work as well:

let providers = [AppService];

@Module({
  imports: [],
  controllers: [AppController],
  providers, <--
})

The error happens when we try to iterate the elements:

        if (optionProperty) {
            return new Set(
                (
                    (optionProperty as unknown as TSESTree.Property)
                        .value as TSESTree.ArrayExpression
                ).elements.map( <--
                    (element) => (element as TSESTree.Identifier).name
                )
            );
        }

Disclamer

I understand that's it's a rabbit hole. You also can import providers array from somewhere or construct it dynamically or whatever. But maybe covering a case when providers array is moved to a variable (which is often done to use it in exports) worth doing.

darraghoriordan commented 2 years ago

Hey thanks for reporting this. I fixed the rule so that it won't error for you with providers using identifiers.

but I didn't add support for following identifiers to discover the providers referenced. Like you said it becomes a bit of a rabbit hole. This rule is already a bit hacky because of the way it has to list all providers in modules and then go parse every file.

I didn't want to add actual TS parsing at that step.

Happy to accept a PR for it though 👍

darraghoriordan commented 2 years ago

I added a note to the readme describing the limitation