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

Improve composition operators #698

Merged

Conversation

Peeja
Copy link
Contributor

@Peeja Peeja commented Aug 2, 2023

This one's a bit beefier than the PRs I've been posting so far. There are a few pieces here that could be accepted or rejected separately, but they all seemed to go together, so I went for the whole thing. Let me know if you'd like me to split this up or resubmit with other changes.

The main goal here was to use R.pipedAsync. First, I had a function which returned a constructed Promise, which wasn't supported. Then, I noticed that the types for R.pipedAsync (as well as R.composeAsync and R.pipeAsync) weren't really checking types the way their synchronous equivalents were: they were just accepting a type parameter to assert at the end of the chain. While I was in there, I simplified the functions in terms of one another, but they can work just as well in a more explicit form if you prefer it that way.

There are two minor breaking changes, as described below. One is likely not to come up; the other is easy to adjust code for.

  • R.pipedAsync works with new Promise(): I've made R.pipedAsync await every returned value, so it doesn't need to inspect the function's .toString() to determine if it should consider it async. awaiting a non-thenable simply returns the value, so this should be the expected behavior. (Incidentally, the docs make it sound like this issue applied to R.composeAsync and R.pipeAsync as well, but it didn't—maybe that was changed at some point without updating the docs?) BREAKING: Code which used non async functions returning thenables will now start awaiting those values instead of passing them on directly. This is unlikely to be something anyone has been expecting to work.

  • R.composeAsync, R.pipeAsync, and R.pipedAsync are type-validated: I've used types akin to these functions' synchronous counterparts to follow the types through the chain. (Note: It is possible to write more thorough types for composition, though it gets bulky. I'll leave you that link in case you're interested at some point.) BREAKING: As demonstrated in mapToObjectAsync-spec.ts, the type parameter signature for these has changed, matching the shape of the synchronous versions. Unfortunately, this is likely to come up for many users of these functions, since they previously depended on this parameter for any type support. Luckily, the change is easy: client code should in many cases now be able to remove the type argument and let TS infer meaningful types, and where client code would prefer to still assert the type of the output, it can use an actual type assertion with as SomeType at the end.

  • R.composeAsync and R.pipedAsync are built on R.pipeAsync: This simply refactors these functions so as not to re-write the looping logic in three places. No behavior should be changed.

  • R.pipeAsync is built on R.reduce: Similarly, this is just a refactoring. No behavior should be changed.

@Peeja Peeja force-pushed the types-through-async-compositions branch 2 times, most recently from 5510ab6 to e8c58e9 Compare August 2, 2023 15:25
* `R.pipedAsync` works with `new Promise()`
* `R.composeAsync`, `R.pipeAsync`, and `R.pipedAsync` are type-validated
* `R.composeAsync` and `R.pipedAsync` are built on `R.pipeAsync`
* `R.pipeAsync` is built on `R.reduce`
@Peeja Peeja force-pushed the types-through-async-compositions branch from e8c58e9 to fd4a761 Compare August 2, 2023 15:35
@selfrefactor
Copy link
Owner

I have to say, this is maybe the best MR this repo had so far in terms of explanation. I love it.

I will take a look, but my initial thoughts are that I will accept all the changes and make major bump to Rambdax. I will keep you posted for the progress of this MR, but it will be merged.

The implementations accepted promises as the original arguments; now the
types do as well.
@Peeja
Copy link
Contributor Author

Peeja commented Aug 7, 2023

Thanks! I've added another commit to fix the types so that the composed functions can accept promises from the beginning.

@selfrefactor
Copy link
Owner

Thanks for the additional commit. I will focus on releasing new Rambda version so we can see your changes in NPM. Afterwards, I will merge this PR. I will keep you informed with the progress, but this weekend I'd expect to be ready.

@Peeja
Copy link
Contributor Author

Peeja commented Aug 8, 2023

Thanks! These functions are modular enough that it's not too hard to have wrappers in my own project with the new types, so don't rush on my account. I can swap them back out whenever the release comes.

@selfrefactor selfrefactor merged commit daae49b into selfrefactor:master Aug 20, 2023
2 checks passed
@selfrefactor
Copy link
Owner

Released to Rambdax, sorry for the delay

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