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

Supports passing a default provider in the inject decorator. #264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

etiennenoel
Copy link
Collaborator

This PR adds the ability to support defining a default provider, directly inside the @inject decorator. Passing both a defaultProvider and isOptional:true, will simply use the defaultProvider, making isOptional: true useless in this case. We could add a warning or error but I think that doing the right thing, which is to use the default Provider is the right call.

Maybe the best thing we should do is use a conditional type like this, so devs can know that it's one or the other.

type InjectOptionsType = {} & (
  | {isOptional?: boolean}
  | {defaultProvider?: Provider<any>});

I think however we would need to increase the version of TypeScript to support this, which I can do in a future version.

This will resolve: #163

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@@ -19,6 +24,12 @@ function inject(
multiple: false,
isOptional: options && options.isOptional
};

if (options && options.defaultProvider) {
// @ts-expect-error options.defaultProvider is the right type but this Typescript version doesn't seem to realize that one of the overloads method would accept this type.
Copy link
Preview

Copilot AI Apr 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring this section to remove the reliance on @ts-expect-error by either upgrading TypeScript or adjusting the overload definitions to properly type-check the defaultProvider.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.


if (options && options.defaultProvider) {
// @ts-expect-error options.defaultProvider is the right type but this Typescript version doesn't seem to realize that one of the overloads method would accept this type.
globalContainer.register(token, options.defaultProvider);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like defaultProvider has the side-effect of making a registration for the token. I'm not sure how I feel about this.

Seems we could get into undefined behavior territory depending on the order things resolve if multiple things use default providers for the same token.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very true. We almost want something like, on resolve, if there is no registration, then check to see if we have a provider in a list of default providers, that we can check into before throwing an error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so this may need to go into the token descriptor, similar to isOptional.

Though I wonder if we could get it working with ES6's default parameters syntax, since I feel most usages of this would be for default values as opposed to factories and the like.

(if we're registering a factory, we may as well have a full fledged registration; though I could be wrong and not seeing the right use case yet)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Factories, I could see wanting easily access to the container at runtime. Let me try to put this in the token descriptor to see if that makes more sense.

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

Successfully merging this pull request may close these issues.

2 participants