Skip to content

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 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.

};

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

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 uses AI. Check for mistakes.

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