microsoft / tsyringe

Lightweight dependency injection container for JavaScript/TypeScript
MIT License
5.19k stars 171 forks source link

Container resolve does work anymore with default parameters since v4.4 #138

Open woutervanbakel opened 4 years ago

woutervanbakel commented 4 years ago

Describe the bug DI does not work properly when using default values.

Error: Cannot inject the dependency at position #0 of "Target" constructor. Reason:
    TypeInfo not known for "Object"

To Reproduce Build and run main.

import "reflect-metadata";
import {container, injectable} from "tsyringe";

@injectable()
class Target {
  constructor(subject = "hello world") {}
}

function main() {
    const t = container.resolve(Target);
}

Expected behavior t correctly resolved with t.subject == "hello world"

Version: tsyringe v4.4

MeltingMosaic commented 4 years ago

Does Target need the @injectable() decorator? It works if you remove the @injectable() decorator, since TSyringe will then use the constructor as written, rather than one that searches the container for tokens.

woutervanbakel commented 4 years ago

@MeltingMosaic Yes sorry you are absolutely right. while I was minimizing the code for an easy example I missed out on the actual issue I faced. I've a class that needs to be a singleton.

import "reflect-metadata";
import {container, singleton} from "tsyringe";

@singleton()
class Target {
  constructor(subject: string = "hello world") {}
}

function main() {
    const t = container.resolve(Target);
}

This results in the following error:

Error: Cannot inject the dependency at position #0 of "Target" constructor. Reason: TypeInfo not known for "String"

It does however seems to work for Objects but not for Primitives. For example this is ok:

import "reflect-metadata";
import {container, singleton} from "tsyringe";

class Subject {}

@singleton()
class Target {
  constructor(public subject: Subject = new Subject()) {}
}

function main() {
    const t = container.resolve(Target);
}

Looks like a bug?

woutervanbakel commented 4 years ago

Because this issue comes from an existing code-base I was able to do some more analysis. I found that if I downgrade to v4.3.0 it works again. Looking at the commits in 4.4.0 I do see some changes that are probably related.

MeltingMosaic commented 4 years ago

I tested this on 4.3.0, and it still fails:

import "reflect-metadata";
import { container, singleton } from "tsyringe";

test("should work", () => {
  @singleton()
  class Target {
    constructor(public subject: string = "hello world") {}
  }

  const t = container.resolve(Target);

  expect(t).toBeTruthy();
});

Could you point me at the code where this works on 4.3.0? It's possible I have not built a representative sample.

In your second case, it does work because the container can construct an undecorated object with a default constructor. However, the initializer will not run since Subject will be injected from the container:

test("should work", () => {
  class Subject {
    public message: string;
    constructor() {
      this.message = "Base Class";
    }
  }

  class SubjectDerivative extends Subject {
    constructor() {
      super();
      this.message = "Derived Class";
    }
  }

  @singleton()
  class Target {
    constructor(public subject: Subject = new SubjectDerivative()) {
      console.log(subject.message);
    }
  }

  const t = container.resolve(Target);

  expect(t).toBeTruthy();
});

Outputs:

    console.log
      Base Class

      at new Target (src/test.ts:22:15)
woutervanbakel commented 4 years ago

Sorry for the late reply but I tested your examples with jest in on a local project and there it worked with 4.3.0. Did you checked your package-lock to confirm you are actually running 4.3? In my case I needed to force npm with


npm install tsyringe@4.3.0
MeltingMosaic commented 3 years ago

Yeah, I forced 4.3 (and verified that the new methods in 4.4 were absent from the JS)

ffflorian commented 3 years ago

Still the same problem with 4.5.0

MeltingMosaic commented 3 years ago

Ok, I have a repro now.

catharsisjelly commented 3 years ago

Hey, I have the same issue happening v4.5 but only once I import azure/service-bus I don't know if this helps or not but might be related somehow.

ljleb commented 3 years ago

Same problem here, are there any known workarounds? I'm using default arguments to enable unit testing in my case

RaulGF92 commented 2 years ago

Still the same problem with 4.6.0

import { inject, injectable } from 'tsyringe';
import config from 'config';
import AWS from 'aws-sdk';
import Database from '../database/Database';

@injectable()
export default class SqsClient {
    constructor(
        private sqs: AWS.SQS = new AWS.SQS(),
        private queueUrl: string = <string> config.get('cu-enablement-queue-url')
    ) {
    }

    sendMessage(message) {
        const params = {
            MessageBody: JSON.stringify(message),
            QueueUrl: this.queueUrl
        };
        return this.sqs.sendMessage(params).promise();
    }
}
Error: Cannot inject the dependency at position #1 of "SqsClient" constructor. Reason:
    TypeInfo not known for "String

EDIT: In my example, if I change the "string" for an Object Tsyring will work soo I will start to think that the problem is possible with Generic Types and don't with classes

RaulGF92 commented 2 years ago

Why is it closed but it's not the best approach?

This is how I solved the problem indeed this solution is in the part of documentation...

https://github.com/microsoft/tsyringe#injecting-primitive-values-named-injection

import { container, inject, injectable } from 'tsyringe';
import config from 'config';
import AWS from 'aws-sdk';

container.register('SqsClientQueueUrl', {
    useValue: <string> config.get('cu-enablement-queue-url')
});

@injectable()
export default class SqsClient {
    constructor(
        private sqs: AWS.SQS = new AWS.SQS(),
        @inject('SqsClientQueueUrl') private queueUrl: string
    ) {
    }

    sendMessage(message) {
        const params = {
            MessageBody: JSON.stringify(message),
            QueueUrl: <string> this.queueUrl
        };
        return this.sqs.sendMessage(params).promise();
    }
}

But I don't think this is the best approach because you are forced to make an extern initialization... Hopefully, the dev team will implement the logical behaviour and we won't need to use that.

semiautomatixfu commented 2 years ago

Yeah, having issues with my @singleton class:

import { singleton } from 'tsyringe';

@singleton()
export class Config {
  public readonly region: string;
  constructor(
    region?: string,
  ) {
    this.region = region || process.env.REGION || 'localhost';
  }
}

Basically I need the optional value outright ignored outside of testing, unit tests call the Config file with the constructor and add it to the container.

RaulGF92 commented 2 years ago

Yeah, having issues with my @singleton class:

import { singleton } from 'tsyringe';

@singleton()
export class Config {
  public readonly region: string;
  constructor(
    region?: string,
  ) {
    this.region = region || process.env.REGION || 'localhost';
  }
}

Basically I need the optional value outright ignored outside of testing, unit tests call the Config file with the constructor and add it to the container.

That is your solution using Tsyring but if you need to create runtime Config instances I don't recommend you use singleton or Tsyringe, use a simple class.

import { singleton, inject } from 'tsyringe';

container.register('ConfigRegion', {
    useValue: process.env.REGION || 'localhost'
});

@singleton()
export class Config {
  public readonly region: string;
  constructor(
    @inject("ConfigRegion") region?: string,
  ) {
    this.region = region;
  }
}
semiautomatixfu commented 2 years ago

Thanks for the input. I'm fairly unfamiliar with Tsyringe, having only moved our DI this week.

But yes, you're probably correct about a simple class, I'm was merely switching the repo to Tsyringe, I'll look at best practices once that's complete.

0xF6 commented 2 years ago

Still in 4.7.0...

0xF6 commented 2 years ago

Why this problem still exists when i register object through .register(type, { useValue: val });?

S1nPur1ty commented 2 years ago

Because there are idiots who don't care at Microsoft.

nosachamos commented 1 year ago

Still having this problem in Feb 2023. Is this lib dead? Seems like a pretty big issue to me - injecting singletons as default arguments in constructors are the main thing I use tsyringe for, but if it doesnt work then I suppose I should move on?

0xF6 commented 1 year ago

Still having this problem in Feb 2023. Is this lib dead? Seems like a pretty big issue to me - injecting singletons as default arguments in constructors are the main thing I use tsyringe for, but if it doesnt work then I suppose I should move on?

Project is dead, do not use it

ArjixWasTaken commented 2 weeks ago

Project is dead, do not use it

works great for me!

RaulGF92 commented 2 weeks ago

Project is dead, do not use it

works great for me!

Library is nice and works fine but maintenance and roadmap for future features is none, really at today I could said there is only one alternative that is InversifyJS but I prefer TSyringe.....

Also to another people who loves TSyringe style take a look to summerjs https://summerjs.dev/ the approach is to similar to tysringe but don't use it.... I discover when I tried to code exactly same framework also called exactly the same