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.

Sign up for GitHub

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

Jump to bottom

Fixed types of Either.fromTry #59

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixed types of Either.fromTry #59

wants to merge 3 commits into from

Conversation

wookieb
Copy link

@wookieb wookieb commented Apr 4, 2023 •
edited
Loading

Addresses comment: #53 (comment)

@wookieb wookieb mentioned this pull request Apr 4, 2023
Merged
@JSMonk
Copy link
Owner

JSMonk commented Jul 16, 2023

Hey. It looks like the typing is good now because you can provide the types by generic types application. Why did you decide to change them?

@JSMonk
Copy link
Owner

JSMonk commented Jul 16, 2023

I saw the issue, but, let's just change the documentation. Could I ask you to do the separated PR?

@wookieb
Copy link
Author

wookieb commented Jul 19, 2023

@JSMonk
I've updated readme within that PR. Did I miss some part of documentation?

@JSMonk
Copy link
Owner

JSMonk commented Jul 19, 2023

No, your PR is great, I just want to split it in two: one with the changes to the readme, and another one with all the logic

@wookieb
Copy link
Author

wookieb commented Jul 19, 2023

I can. What's the benefit of it though?

@JSMonk
Copy link
Owner

JSMonk commented Jul 19, 2023

Seems like that in the case of throwing an error inside the function provided to the fromTry function, the type of the error will be not inferenced by TypeScript, so, this is the issue of the documentation.
I would like to fix it separately (I could do it by myself), but removing of the generic parameter it's another task that I would like to discuss before the merging (at least, it will be great to not remove the parameter, but provide the unknown as a default value for the type parameter)

konradekk
konradekk reviewed Jul 19, 2023
View reviewed changes
Copy link

@konradekk konradekk 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 about those (donʼt have much time to check it), not sure if extending unknown is sensible or not really… (i.e. to go with L extends unknown = unknown but probably not many types naturally extend unknowns…!)

either/index.ts Outdated
@@ -193,11 +193,11 @@ class EitherConstructor<L, R, T extends EitherType = EitherType>
return EitherConstructor.right(v);
}

static fromPromise<L, R>(promise: Promise<R>): Promise<Either<L, R>> {
static fromPromise<R>(promise: Promise<R>): Promise<Either<unknown, R>> {
Copy link

Choose a reason for hiding this comment

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

you could probably also go for something like this:

Suggested change
static fromPromise<R>(promise: Promise<R>): Promise<Either<unknown, R>> {
static fromPromise<R, L = unknown>(promise: Promise<R>): Promise<Either<L, R>> {

not sure of all the consequences, though!

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

either/index.ts Outdated
return promise.then(EitherConstructor.right).catch(EitherConstructor.left);
}

static fromTry<L, R>(fn: () => R): Either<L, R> {
static fromTry<R>(fn: () => R): Either<unknown, R> {
Copy link

Choose a reason for hiding this comment

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

once again,

Suggested change
static fromTry<R>(fn: () => R): Either<unknown, R> {
static fromTry<R, L = unknown>(fn: () => R): Either<L, R> {

is an option…? 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

This is exactly what I propose.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@wookieb wookieb force-pushed the either-from-try-types branch from c0bd847 to 93542bf Compare July 23, 2023 06:21
@wookieb wookieb mentioned this pull request Jul 23, 2023
Open
@wookieb
Copy link
Author

wookieb commented Jul 23, 2023 •
edited
Loading

Docs in another PR: #66

JSMonk
JSMonk reviewed Jul 26, 2023
View reviewed changes
either/index.ts
@@ -193,11 +194,11 @@ class EitherConstructor<L, R, T extends EitherType = EitherType>
return EitherConstructor.right(v);
}

static fromPromise<L, R>(promise: Promise<R>): Promise<Either<L, R>> {
static fromPromise<R, L = unknown>(promise: Promise<R>): Promise<Either<L, R>> {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we save the previous order of params by adding the unknown default value to the R param too?

Copy link
Author

@wookieb wookieb Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That would be counter intuitive since whenever working with promises most of the the time you care about the result which is now first generic argument. From the other side this is "breaking" change.

Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

It's a breaking change. I will put it into the new major version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konradekk konradekk konradekk left review comments

@JSMonk JSMonk JSMonk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wookieb @JSMonk @konradekk