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

feat(types): functions must be wrapped in functions #7

Merged

Conversation

marcvincenti
Copy link
Contributor

When a provider require an injectable extending a function signature, this function needs to be wrapped by another function since the module will automatically call the former function to instanciate the injectable.

Dependencies can be of type T | (args?: any) => T, we propose to narrow this type to only (args?: any) => T when T is a function.

src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated
/**
* Factory as defined in the injectable property: either a function with eventually a single named dependencies argument object or a value
*/
export type FactoryFn<FactoryLike> = FactoryLike extends (args: any) => any
export type FactoryFn<FactoryLike> = FactoryLike extends Function

Choose a reason for hiding this comment

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

it is not quite the same. Factories have only one argument

Copy link
Contributor Author

@marcvincenti marcvincenti Oct 7, 2024

Choose a reason for hiding this comment

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

Yes, i took an unnecessary shortcut by sharing a generic Function type. I removed it from this PR.
Edit: and wrong shortcut

src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated Show resolved Hide resolved
@LaurentAtGreenly
Copy link

Looks good Marc. Thanks for the contribution

@marcvincenti
Copy link
Contributor Author

Thank you for your review and all your propositions. 🙏
I was having a hard time to name the new type WrapFunctionInjectable 😅
I have integrated them all in a review commit.

@marcvincenti marcvincenti marked this pull request as draft October 7, 2024 11:41
@marcvincenti
Copy link
Contributor Author

marcvincenti commented Oct 7, 2024

EDIT: fixed and I added a test for this case

I'm noticing a weird behavior when a provider require a dependency being directly an union type (it works fine with a function that return an union type).

I cannot inject the dependency wrapped in a function because of a weird TS behavior that split the type of (deps: any) => T.

Example:

createProvider({
  injectables: {
    foo: ({ bar }: { bar: 'a' | 'b' }) => bar,
    bar: (): 'a' | 'b' => 'a', // <-- Type '() => "a" | "b"' is not assignable to type '"b" | "a" | ((deps?: any) => "b") | ((deps?: any) => "a")'.
  },
  api: ['foo'],
});

@marcvincenti marcvincenti marked this pull request as ready for review October 7, 2024 11:59
Copy link
Owner

@lorenzofox3 lorenzofox3 left a comment

Choose a reason for hiding this comment

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

Not sure why you use an array. But, indeed, your test with the union fails if you don't 😅

@lorenzofox3 lorenzofox3 merged commit 0f14724 into lorenzofox3:main Oct 19, 2024
1 check passed
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.

4 participants