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

Error thrown on moduleRef.resolve() is a breaking change #4866

Closed
janhartigan opened this issue Jun 1, 2020 · 6 comments
Closed

Error thrown on moduleRef.resolve() is a breaking change #4866

janhartigan opened this issue Jun 1, 2020 · 6 comments
Labels
needs triage This issue has not been looked into

Comments

@janhartigan
Copy link

Bug Report

Current behavior

Calling moduleRef.resolve() on non-transient providers throws an error when it did not before 7.1.2.

The relevant commit is:

439736f

Input Code

import { Injectable } from '@nestjs/common';
import { ModuleRef } from '@nestjs/core';

import { MyClass } from './MyClass';

@Injectable()
export class Foo {
  constructor(private readonly moduleRef: ModuleRef) {}

  async build(): Promise<MyClass> {
    return await this.moduleRef.resolve(MyClass);
  }
}

Expected behavior

I understand that resolve should only be called for transient-scoped providers, but before this update, it did not throw an error when not doing so. This is a breaking change introduced in a patch version. I have full coverage in my repos, so I was able to catch this, but for anyone who is relying on Nest not to release breaking changes in minor updates, this will be a problem.

Possible Solution

Revert the change. Re-introduce it in a major version update.

@maximelkin
Copy link

I understand that resolve should only be called for transient-scoped providers

Why default scoped should not be allowed?
I want setup app global interceptors/guards, and for this I need to resolve all dependencies of them, but, since they are Scope.DEFAULT, it's not working.

@alexgarbarev
Copy link

alexgarbarev commented Jun 7, 2020

Same problem. I understand that resolve was changed to support "only scoped" providers, to add clarity and predictability, removing ambiguity.
At same time, there are cases, when we don't know scope of provider unless runtime. For example I might design architecture, where I dispatch to providers dynamically regarderless their scope (scope would be defined by providers authors).
If that resolve method usage is wrong, then we need an alternative that would resolve provider with any scope.

Also, we need a better exception message at least, now it says "... is marked as a scoped provider ... Please, use "resolve()" instead", but

  1. my providers scope set explicitly to DEFAULT, so not sure what does means "scoped" - everything is scoped then.
  2. I'm using "resolve" method

As a workaround I had to set all these providers to TRANSIENT scope.

Thanks for a great framework btw.

@janhartigan
Copy link
Author

@maximelkin The reasoning behind this makes sense given how Nest's injector works behind the scenes. The idea is that resolve is intended to be used to force-resolve a dependency chain that has some non-global-scoped providers in it. In the Nest world, these kinds of providers don't get instantiated until the a request comes in, so it makes sense that you might need an async method to do the work. get is intended to be used for globally-scoped providers.

I think a case could be made for having a method available that does either/or...it's not always easy to know when a particular provider is request scoped or not (although frequently this is a red flag for abusing the provider scopes).

@maximelkin
Copy link

@janhartigan thank you! My bad, not carefully read the docs

@MichalLytek
Copy link

MichalLytek commented Jun 10, 2020

I've also encountered recently this issue while upgrading the dependencies to a minor version.

The error is really confusing, because I always call .resolve() and now for the global scoped providers, it tells me:

RecipeResolver is marked as a scoped provider.
Request and transient-scoped providers can't be used in combination with "get()" method.
Please, use "resolve()" instead."

If the real intent is to allow .get() only for global scoped providers and .resolve() for transient/request scoped, the message has to be updated because I got use "resolve()" instead while using .resolve() 😄

For all the usage mentioned here, the fix is simple - read the ModulesContainer metadata and make a proper if(wrapper.isDependencyTreeStatic() && !wrapper.isTransient) to call .get(). or .resolve() properly 😉
MichalLytek/typegraphql-nestjs@7acd23d

@kamilmysliwiec
Copy link
Member

Since there's no easy way to dynamically determine the scope of a provider, I've reverted this breaking change for now (+ error message was misleading either way 😅 ). Fixed in the latest release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

5 participants