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

Wrong typescript definition for "when" method #317

Open
hasfoug opened this issue Mar 29, 2023 · 3 comments · May be fixed by #318
Open

Wrong typescript definition for "when" method #317

hasfoug opened this issue Mar 29, 2023 · 3 comments · May be fixed by #318

Comments

@hasfoug
Copy link

hasfoug commented Mar 29, 2023

Hi there,

I've noticed that the typescript definition for "when" method is wrong

when(condition: boolean, fn: (this: any) => any, defaultFn: (this: any) => any): void;

In reality, the first callback function is most likely gonna return the modified collection, so I assume the return type of the method should be the Collection itself.

The current definition makes the compiler think that chained methods after the "when" method call are called on void, while in reality, it is not true because they are can be called on the modified collection.

E.g.

collect(items).when(condition1, (collection) => collection.push(newItem)).when(condition2, (collection) => collection.pop())

I suppose that void also makes sense sometimes though, so it would be good if the return type is

Collection<Item> | void

Also, the second callback defaultFn should be optional, right now it is required in the typescript definition which also makes the compiler throw warnings.

To sum up, the right definition in my opinion should be next:

when(condition: boolean, fn: (this: any) => any, defaultFn?: (this: any) => any): Collection<Item> | void;

@ecrmnn
Copy link
Owner

ecrmnn commented Mar 31, 2023

Hi, @hasfoug.

I'm not seasoned enough in TypeScript to comment on this.
If this is the correct definition I'd be happy to merge a pull request.

@hasfoug
Copy link
Author

hasfoug commented Apr 20, 2023

@ecrmnn

Thanks for your reply, I've created the PR with the changes #318

@hasfoug
Copy link
Author

hasfoug commented Jun 2, 2023

Hi @ecrmnn,

Could you merge it?

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 a pull request may close this issue.

2 participants