DIContainer swallows errors inside ConstructorDeclarations

[wessberg]

DIContainer attempts to invoke an implementation with new, and if that throws, it attempts again without new.
It has an implicit expectation that if the constructor fails, it will be because it must be called as a function without new.
However, there can be many reasons why the constructor produced an exception.

You can see the this problem at line 236 in di-container.ts.

[cmidgley]

I ended up taking an approach that used the error caught in the first (and primary) attempt to instantiate the object, and then when the second (backup) attempt fails, throw the original error. For me, this is working well.

try {
	// Try to construct an instance with 'new' and if it fails, call the implementation directly.
	const newable = registrationRecord.implementation as NewableService<T>;
	instance = new newable(...instanceArgs);
} catch (ex) {
	if (registrationRecord.implementation == null) {
		throw new ReferenceError(
			`${this.constructor.name} could not construct a new service of kind: ${identifier}. Reason: No implementation was given!`
		);
	}
	const constructable = registrationRecord.implementation;
	// Try without 'new' and call the implementation as a function.
	try {
		instance = (constructable as unknown as CallableFunction)(...instanceArgs);
	} catch {
		throw ex;
	}
}

[firstpilotpirx]

Interface

export interface HelloService {
    hello(): void;
}

Implementation

import {HelloService} from "./hello.service";

export class HelloOverConsoleLogService implements HelloService {
    constructor() {
        throw new Error('Error in constructor!!!');
    }

    hello(): void {
        console.log('Hello, World!');
    }
}

Setup DI

import {DIContainer} from "@wessberg/di";
import {HelloService} from "./services/hello.service";
import {HelloOverConsoleLogService} from "./services/hello-over-console-log.service";


async function main(): Promise<void> {
    const container = new DIContainer();
    container.registerTransient<HelloService, HelloOverConsoleLogService>();
    const helloService = container.get<HelloService>();
    helloService.hello();
}

main()
    .then(() => {
    })
    .catch((error) => {
        console.error('main function error', error);
    });

Logs

main function error TypeError: Class constructor HelloOverConsoleLogService cannot be invoked without 'new'

Description

Sometimes constructors do db, queue or cache connection, sometimes constructors load cache from file or just have peace of logic and this logic has errors, as usually, it happens everywhere and always.

And where we have bug in our constructor we would like to see the error logs. exception stack trace etc. But if we see cannot be invoked without 'new', we really do not have ideas what was wrong and where.

Is it posible to show all error what happens while DI create objects ? It would be really usfull.

[cmidgley]

This is what my pull request attempts to improve on, to give the original error back rather than obscuring it (for the most common use case where a class constructor is used for the factory). What's happening is internally in DI it attempts to create the object with new, and if it fails it falls back on trying to call it as a function. It is that second error that is then thrown (not having new). In most cases, the first error (when new was the correct action but the constructor threw an error), is the one you really want.

One solution is to merge my pull request. You can also directly change DI/di-container.ts file (here as stated above). It isn't a perfect solution, as those cases where you want to use a non-constructor method would have their thrown error obscured, but for me, it solved the problem.

[firstpilotpirx]

Yes, I see. I also did fork of the project and a little bit modified code. I just add log for this error, it help me debug this situation.

If I understand the problem right, I think more clean solution is split creating object from class constructor and from factory function. Becase one part of code create object from constructor and from factory function and there are not information about what is it: constructor or function.

[cmidgley]

I just checked the code... and I see in the constructInstance method it first handles the factory function construction. And by the time it reaches the code we are talking about, it would no longer have a factory function but just a regular constructor.

So I'm unclear what the use case is that drives this second condition (the need to "call the implementation directly"). I'm wondering if maybe this is left over from an old implementation for factory functions and is no longer needed. Is it possible the correct solution is to replace:

        const constructable = registrationRecord.implementation;
        // Try without 'new' and call the implementation as a function.
        instance = (constructable as unknown as CallableFunction)(
          ...instanceArgs
        );

with

    throw ex;