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

Align the logic of isOptionalFactoryDependency internal utility #14154

Closed
1 task done
micalevisk opened this issue Nov 17, 2024 · 2 comments · Fixed by #14175
Closed
1 task done

Align the logic of isOptionalFactoryDependency internal utility #14154

micalevisk opened this issue Nov 17, 2024 · 2 comments · Fixed by #14175

Comments

@micalevisk
Copy link
Member

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

at @nestjs/common we have a x is OptionalFactoryDependency with the following logic:

function isOptionalFactoryDependency(
x: InjectionToken | OptionalFactoryDependency,
): x is OptionalFactoryDependency {
return !!((x as any)?.token && !(x as any)?.prototype);
}

but at @nestjs/core we have a x is OptionalFactoryDependency with a bit different logic:

const isOptionalFactoryDep = (
item: InjectionToken | OptionalFactoryDependency,
): item is OptionalFactoryDependency =>
!isUndefined((item as OptionalFactoryDependency).token) &&
!isUndefined((item as OptionalFactoryDependency).optional);

I can't tell if this was intentional or not. Thus, I can't tell which function has the correct logic.

Describe the solution you'd like

Ensure that both isOptionalFactoryDependency and isOptionalFactoryDep have the same logic, as we don't want to expose that internal utility from outside of @nestjs/* packages.

Teachability, documentation, adoption, migration strategy

N/A

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

I believe that OptionalFactoryDependency should be just one thing.

Also, 3rd-party packages might want to reuse that logic (like nestjs-spelunker here: https://github.com/jmcdo29/nestjs-spelunker/blob/3cf07cbaef306e885eb031ad46f979cc7497dfd1/src/exploration.module.ts#L149-L153).

@kamilmysliwiec
Copy link
Member

Sounds good to me. Would you like to create a PR for this issue?

@micalevisk micalevisk moved this from 🤔 I'll investigate later to 🚧 Working on this! in My contributions to NestJS framework 😻 Nov 18, 2024
@micalevisk micalevisk removed the needs triage This issue has not been looked into label Nov 20, 2024
@kamilmysliwiec
Copy link
Member

let's track this here #14175

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

Successfully merging a pull request may close this issue.

2 participants