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

fix: TypeDI integration not working as expected on docs #642

Open
Leafgard opened this issue Jan 12, 2021 · 16 comments · Fixed by #643
Open

fix: TypeDI integration not working as expected on docs #642

Leafgard opened this issue Jan 12, 2021 · 16 comments · Fixed by #643
Labels
flag: needs docs Issues or PRs which still need documentation added. status: wontfix

Comments

@Leafgard
Copy link
Contributor

Description

Cannot use TypeDI as explained on the README for basic services.

Obviously, it is written that it only requires the @Service on service side (seems legit), but this actually doesn't work without telling @Service before the controller as well.

Minimal code-snippet showcasing the problem

Controller snippet

import { Service } from 'typedi'

import { Get, JsonController, Param, State } from 'routing-controllers'
import { CollaboratorService } from '../services/CollaboratorService'

@JsonController('/service/:serviceId/collaborator')
@Service() // This shouldn't be required as described on the README, but it is actually required to run the app as expected
export class CollaboratorController {
  constructor(private collaboratorService: CollaboratorService) { }

  @Get('/')
  async getCollaborators (@State('user') user: User, @Param('serviceId') serviceId: number) {
    return await this.collaboratorService.getCollaborators(
      user.id,
      serviceId
    )
  }

Actual service snippet

import { Service } from 'typedi'

@Service()
export class CollaboratorService {
  async getCollaborators (userId: number, serviceId: number) {
    ...
  }
}

Expected behavior

I've imported everything and created the DI Container before the app, the Controller should be working without the @Service decorator declaration.

Actual behavior

It doesn't, if I try to run the code without the @Service decorator on top of the Controller, I'll get the following error:

{
    "name": "ServiceNotFoundError",
    "message": "Service with \"MaybeConstructable<CollaboratorService>\" identifier was not found in the container. Register it before usage via explicitly calling the \"Container.set\" function or using the \"@Service()\" decorator.",
    "stack": "ServiceNotFoundError: Service with \"MaybeConstructable<CollaboratorService>\" identifier was not found in the container. Register it before usage via explicitly calling the \"Container.set\" function or using the \"@Service()\" decorator.\n    at ContainerInstance.get (L:\\...\\node_modules\\typedi\\cjs\\container-instance.class.js:45:15)\n    at Object.value (L:\\...\\node_modules\\typedi\\cjs\\decorators\\inject.decorator.js:31:42)\n    at L:\\...\\node_modules\\typedi\\cjs\\container-instance.class.js:286:58\n    at Array.forEach (<anonymous>)\n    at ContainerInstance.applyPropertyHandlers (L:\\...\\node_modules\\typedi\\cjs\\container-instance.class.js:280:46)\n    at ContainerInstance.getServiceValue (L:\\...\\node_modules\\typedi\\cjs\\container-instance.class.js:240:18)\n    at ContainerInstance.get (L:\\...\\node_modules\\typedi\\cjs\\container-instance.class.js:29:25)\n    at Function.get (L:\\...\\node_modules\\typedi\\cjs\\container.class.js:28:36)\n    at Object.getFromContainer (L:\\...\\node_modules\\routing-controllers\\container.js:40:42)\n    at ControllerMetadata.getInstance (L:\\...\\node_modules\\routing-controllers\\metadata\\ControllerMetadata.js:26:28)",
    "normalizedIdentifier": "MaybeConstructable<CollaboratorService>"
}

I don't know if this is an upstream issue or not.

@Leafgard Leafgard added status: needs triage Issues which needs to be reproduced to be verified report. type: fix Issues describing a broken feature. labels Jan 12, 2021
@NoNameProvided NoNameProvided added flag: needs docs Issues or PRs which still need documentation added. status: wontfix type: fix Issues describing a broken feature. and removed status: needs triage Issues which needs to be reproduced to be verified report. type: fix Issues describing a broken feature. labels Jan 12, 2021
@NoNameProvided
Copy link
Member

Hi!

This is the expected behavior. Since TypeDI 0.9.0 it won't create instances for unknown classes, so you need to decorate your classes. This needs to be documented and also I believe a fix is possible as routing-controller can auto-register them in its own decrator.

@Leafgard
Copy link
Contributor Author

Hey

Thanks for the answer, I'll make a pull request tonight to add some documentation about this !

@maurocolella
Copy link

I hate to ask, but: how does something like this sound when the developer says it out loud?

  • Why should I have to mark all participating classes as services?
  • Why does typedi break dependent libraries on "illegal" instantiation? If this is a preference, rather than a behavior critical to the pattern, shouldn't it warn and gracefully return undefined, so that I decide if the missing dependency is critical to me or not?

A meme for the ages.

Services everywhere.

@blackshot
Copy link

Still no solution yet for not having to put @service() on all classes?

@yakovets-evgeny
Copy link

Hi, I have the same problem now, how do I solve it?

/Users/yacovets/Documents/GitHub/Chat-react-socket.io-nodejs/node_modules/src/container-instance.class.ts:75
    throw new ServiceNotFoundError(identifier);
          ^
ServiceNotFoundError: Service with "MaybeConstructable<ErrorHandlerMiddleware>" identifier was not found in the container. Register it before usage via explicitly calling the "Container.set" function or using the "@Service()" decorator.
    at ContainerInstance.get (/Users/yacovets/Documents/GitHub/Chat-react-socket.io-nodejs/node_modules/src/container-instance.class.ts:75:11)

NodeJs: 16.0.0
"typedi": "^0.10.0"
"reflect-metadata": "^0.1.13"
"routing-controllers": "^0.9.0"

index.ts

import 'reflect-metadata'
import { useExpressServer, useContainer } from 'routing-controllers'
import { Container } from 'typedi'

import './util/env'
import Server from './core'
import socket from './socket'
import * as controller from './controller'
import { ErrorHandlerMiddleware, ErrorNotFoundMiddleware } from './middleware'
import './service'

useContainer(Container)

const server: Server = new Server()
socket(server.getIo())

useExpressServer(server.getApp(), {
    routePrefix: '/v1',
    controllers: controller.v1,
    defaultErrorHandler: false,
    middlewares: [
        ErrorHandlerMiddleware,
        ErrorNotFoundMiddleware
    ]
})

server.listen()

export default server

controller.ts

import { Response } from 'express'
import { JsonController, Post, Req, Res, UseBefore } from 'routing-controllers'
import ModifiedRequest from '../../interface/ModifiedRequest'
import { CheckAuthorizationMiddleware } from '../../middleware/CheckAuthorizationMiddleware'
import {UserService} from "../../service/UserService";
import {Service} from "typedi";

@JsonController()
@Service()
@UseBefore(CheckAuthorizationMiddleware)
export default class User {

    constructor(private userService: UserService) {}

    @Post('/profile')
    getProfile(@Req() request: ModifiedRequest, @Res() response: Response) {
        return this.userService.getProfile()
    }

}

service.ts

import { Service } from 'typedi'

@Service()
export class UserService {

    getProfile() {
        return {
            ok: true
        }
    }

}

@Leafgard
Copy link
Contributor Author

Leafgard commented May 8, 2021

@jenya-yacovets Please take some time to investigate before asking someone to do your job. 😉

It seems like your ErrorHandlerMiddleware "Service" isn't found in Container, but since you didn't provide any part of this code, I can just guess that you forgot adding the @Service() decorator in front of your middleware !

@yakovets-evgeny
Copy link

@jenya-yacovets Please take some time to investigate before asking someone to do your job. 😉

It seems like your ErrorHandlerMiddleware "Service" isn't found in Container, but since you didn't provide any part of this code, I can just guess that you forgot adding the @Service() decorator in front of your middleware !

Thanks! I thought that only in controllers you need to specify @service()

@guidsdo
Copy link

guidsdo commented Sep 19, 2021

This is what I'm using to save me from writing more code than necessary 😁 :

export function ServiceController(...args: Parameters<typeof Controller>) {
    return <TFunction extends Function>(target: TFunction) => {
        Service()(target);
        Controller(...args)(target);
    };
}

export function ServiceJsonController(...args: Parameters<typeof Controller>) {
    return <TFunction extends Function>(target: TFunction) => {
        Service()(target);
        JsonController(...args)(target);
    };
}

@slavafomin
Copy link
Contributor

I had to update ~60 classes to add @Service everywhere. This is just crazy. Couldn't @Controller decorator automatically register controller/middleware classes with the provided DIC? I guess, this should be just a couple of lines of code in the library.

@aaarichter
Copy link

it is also odd how semver is handled in the related libs - breaking changes like this require a major bump or a graceful behavior as stated in the comment #642 (comment)

@bvanreeven
Copy link

@aaarichter In SemVer, going from version 0.x.y to 0.x+1.0 is considered a major bump. NPM also handles it as such, e.g. depending on typedi@^0.8.0 would install the latest 0.8.x version, not 0.9.x.

@attilaorosz
Copy link
Member

@NoNameProvided I believe this has been solved. Package is working as expected. I agree that controllers should maybe register themselves in the DI without the explicit @service decorator but that's a feature request at this point.

@NoNameProvided
Copy link
Member

Let's keep this open for tracking. I think we may auto-register services, but we need to discuss this further.

@attilaorosz attilaorosz removed the type: fix Issues describing a broken feature. label Feb 27, 2022
@kenberkeley
Copy link

kenberkeley commented Nov 22, 2022


Update: they are the same... even TSyringe still require us to apply @autoInjectable() on the controller level...

@5E7EN
Copy link

5E7EN commented Jun 9, 2023

Any update on this?
Has there been a decision on whether to support auto registering services? What would be the potential downsides?

@attilaorosz
Copy link
Member

The issue with auto registering is that we have to commit to one di lib, otherwise we cannot really register the controllers because each lib instantiates the services differently. One solution could be to provide a configurable function in the factories where you can set how you want your controllers instantiated, but we would have to resolve the dependencies ourselves then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: needs docs Issues or PRs which still need documentation added. status: wontfix
Development

Successfully merging a pull request may close this issue.