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

OrElse callback should be able to change Ok type #484

Merged
merged 11 commits into from
Sep 8, 2024

Conversation

braxtonhall
Copy link
Contributor

@braxtonhall braxtonhall commented Jun 29, 2023

Unfortunately this would be a breaking change, as anyone who explicitly annotated the types like orElse<A>(foo) will need to update their code to orElse<U, A>(foo)

@braxtonhall braxtonhall marked this pull request as ready for review June 29, 2023 13:28
@supermacro
Copy link
Owner

#496

@myftija
Copy link

myftija commented Nov 4, 2023

@supermacro Any updates on this? Would be really great to have this behavior for orElse :)

@jdisho
Copy link

jdisho commented Dec 24, 2023

+1

Copy link

changeset-bot bot commented Aug 20, 2024

🦋 Changeset detected

Latest commit: 378b3e5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
neverthrow Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@braxtonhall
Copy link
Contributor Author

braxtonhall commented Aug 20, 2024

It is possible to make this PR not a breaking change

- orElse<U, A>(f: (e: E) => Result<U, A> | ResultAsync<U, A>): ResultAsync<U | T, A>
+ orElse<A, U = T>(f: (e: E) => Result<U, A> | ResultAsync<U, A>): ResultAsync<U | T, A>

That way anyone who has already explicitly annotated the type arguments res.orElse<Foo>(..) will have the same behaviour as before.

The downside is of course that this is really awkward to use, especially as it swaps the order or the ok value type and the err value type. I would guess that would be super error prone and surprising to users.

@supermacro
Copy link
Owner

supermacro commented Aug 28, 2024

I think it's OK for this to be implemented as a breaking change.

Actions:

  • please include a changeset file
  • resolve merge conflicts
  • update docs here and here

@braxtonhall
Copy link
Contributor Author

I think it's OK for this to be implemented as a breaking change.

Actions:

  • please include a changeset file
  • resolve merge conflicts
  • update docs here and here

Done! (i think)

Copy link
Collaborator

@m-shaka m-shaka left a comment

Choose a reason for hiding this comment

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

LGTM!

@supermacro if the PR is merged, changesets will create the release PR of v8.0.0.
I'll leave the timing of the merge up to you.

@supermacro supermacro merged commit 2d94df3 into supermacro:master Sep 8, 2024
8 checks passed
@github-actions github-actions bot mentioned this pull request Sep 8, 2024
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.

orElse returns the wrong type when the callback changes the ok type orElse should accept new Ok types
5 participants