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

Incorrect Controller initialization when using "import as" on same controller type from different folders #789

Closed
Masterrg opened this issue Jun 18, 2018 · 12 comments

Comments

@Masterrg
Copy link

Masterrg commented Jun 18, 2018

I'm submitting a...


[ ] Regression 
[X] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

I've got two controllers with the same name (i.e. MainController) in two different folders:
Controller 1: sectionOne/MainController
Controller 2: sectionTwo/MainController

When I am trying to initialize both controllers within a module by using "import as", only one controller gets loaded.

Expected behavior

Both controllers should get initialized.

Minimal reproduction of the problem with instructions

nestjs-bug.zip

npm install
npm start
Result: Only one controller gets initialized.

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

Wouldn't be that bad to be able to reuse controllernames/types from different paths.

Environment


Nest version: ^5.0.1

 
For Tooling issues:
- Node version: 8.9.4
- Platform:  Windows

Others:

@kamilmysliwiec
Copy link
Member

Why would you like to use the exact same controller twice in a single module?

@natqe
Copy link

natqe commented Jun 18, 2018

@kamilmysliwiec his meaning is the exact same name, not the controller himself.

@kamilmysliwiec
Copy link
Member

Isn't is quite confusing when you have two classes with the same name?

@Masterrg
Copy link
Author

Masterrg commented Jun 18, 2018

Not really, especially if you take the whole import path / namespace into consideration.

For example the controller of webhooks\github\main.controller.ts could do something different like webhooks\bitbucket\main.controller.ts - as they are two different things (especially in terms of inheritance) and possibly do different things.

Long story short: Just taking the name of a class/controller into consideration doesn't feel like the correct way in terms of oop. Maybe I am a bit too spoiled by Java or C# or even PHP but overall... it's a bit smelly if "duplicate" class names in different paths/namespaces result in such a conflict.

@kamilmysliwiec
Copy link
Member

Anyway, as far as I know, the same name may cause issues only inside the same module. And putting the classes with the same name into one module is definitely considered as a bad practice. In your ^ example, you could split github and bitbucket directories into two different modules and it'll work fine.

@Masterrg
Copy link
Author

Masterrg commented Jun 19, 2018

I wouldn't agree that it's a bad practice as it's clear in terms of the import path of the specific class and by using import as - using a class A\B\C\MainClass and D\E\F\MainClass doesn't mean that both "MainClasses" got the same type.

It's a similar situation with the github and bitbucket example from above: I would have a webhooks module which references to the specific controllers, as creating a module just for i.e. 2 routes per controller without a lot business logic is not really worth it and may seems to be a bit overloaded.

Nevertheless, I totally got your point.

Suggestion in case the behaviour won't change in future:

  • Maybe extend the documentation and add a part that you only use a "controllername" once in a module
  • Maybe throw an exception in such a case as it's hard to debug if a controller fails silently/doesn't get initialized without any extra information

We experienced this issue after a bigger refactoring session, and all out of sudden some routes weren't available anymore. Luckily we found the issue in a relatively short amount of time - nevertheless you get a small heart attack if things stop working out of sudden without proper information.

@kamilmysliwiec
Copy link
Member

Maybe throw an exception in such a case as it's hard to debug if a controller fails silently/doesn't get initialized without any extra information

We shall at least show a proper warning message. Thanks for reporting, it will be available soon 🙂

@xjrcode

This comment has been minimized.

@imagine10255
Copy link

Does this problem still exist? This problem is the same as currently using 7.0.7.
Is there a way to use namespaces like php laravel?

@wishcn
Copy link

wishcn commented Aug 3, 2020

I met, relationship (in services):

A -> B -> B (parent)
B

Parent B cannot inject child B,
What is the current quick solution?

@stevenhair
Copy link

@xxstop There are two quick fix solutions that I can think of:

  1. If you control one of the classes, just rename it
  2. If you don't control the classes (if they come from third-party libraries, for example), you can extend them and then use your subclass (note that you will need to use the subclass everywhere) - e.g.:
    import { Client } from 'some-package';
    
    export class MyWrappedClient extends Client {}

Neither are really ideal, although it's probably a bad idea to have two classes with the same name in a single project. I inherited a non-Nest project like that and it makes working on the project a bit more difficult - I find myself opening the wrong file a lot.

@kamilmysliwiec
Copy link
Member

Let's track this here #6141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants