nest: feat(core): Implicitly throw an error when someone provides not a module to imports array

Is there an existing issue that is already proposing this?

I have searched the existing issues

Is your feature request related to a problem? Please describe it

Nest.js silently accepts non-module in imports array. e.g.

@Module({
  imports: [SomeClassThatIsNotModule],
})
export class TasksModule {}

Describe the solution you’d like

Nest.js should throw an error when a developer makes an obvious mistake and tries to import anything that is not a module.

Teachability, documentation, adoption, migration strategy

Though throwing an error is not backward compatible, but taking into consideration that most applications are written correctly the change should be compatible.

No documentation updates are needed, the change happens under the hood of the framework.

What is the motivation / use case for changing the behavior?

Making Nest.js framework more reliable framework and enhance developers quality of life.

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 24 (24 by maintainers)

Most upvoted comments

If we go with that idea as well, instead of having some generic “You’ve added a non-module to the imports array” we could be much more specific with “You’ve added a provider with @Injectable() metadata to the imports array. Please remove <insert class name here>”

jmcdo29 on Oct 15, 2021

I think we might want to consider looking at this issue from a slightly different perspective. IF the most common issue you’ve seen is that ppl tend to put services into the imports array, perhaps, we could update the @Injectable() to attach metadata to the host class, and then in the #addModule method, we could check whether the class has this specific metadata assigned and if so, inform the user that "classes annotated with the @Injectable() decorator (providers and all enhancers like guards/interceptors, etc) shouldn’t be located in the imports array.

kamilmysliwiec on Oct 15, 2021

That sounds great, until you realize that a dynamic module’s return is just an Object, so there’s not a blanket “Every module is a class” statement. Plus, you’ll also have to worry about forwardRef fed modules

jmcdo29 on Oct 12, 2021

@micalevisk sounds good to me!

kamilmysliwiec on Dec 13, 2021

This is looking great! Thanks for the work on it

jmcdo29 on Dec 12, 2021

Seems like this feat will increase start time, thus it will be better if this comes disabled by default, right?

Makes sense to me.

also, you’ll have to deal with circular (or bad) imports like:

Hmm, great call-out. I think it will require some book checking on what we already met. 🤔

Also, I was thinking more about that. Potentially this can be done by means of some ESLint custom plugin. 🤖

Ayzrian on Oct 14, 2021