Skip to content

feat: add generic type parameter to using for the resource #6501

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jennings
Copy link

@jennings jennings commented Jul 2, 2021

This overload alleviates the observableFactory from needing to cast its parameter to a more specific type.

Description:

Previously, a TypeScript user has to cast the resource to the appropriate type in order to do anything with it:

const o: Observable<MyResource> = using(
  () => new MyResource(),
  r => of(r as MyResource)   // t is `Unsubscribable | void`, so a cast is needed
)

Adding a type parameter for the resource lets TypeScript infer the type of r more precisely.

I asked in discussion about this and there didn't seem to be a good reason not to implement it.

BREAKING CHANGE (maybe): @cartant suggested this might be a breaking change. There's no functionality change, but there's a worry that it will cause existing code to no longer type-check. The existing signature is in place, so I think at worst it will remove the void from the first parameter's type. That doesn't seem breaking to me, but the maintainers here have definitely seen more weird cases than I have.

Related issue (if exists):

@benlesh
Copy link
Member

benlesh commented Aug 13, 2021

You'll need to run npm run api_guardian:update and commit the new golden files to fix the CI errors.

@jennings jennings force-pushed the add-resource-to-using-type-signature branch from ebfac56 to 296ec78 Compare August 13, 2021 23:52
@jennings
Copy link
Author

@benlesh Thanks, I added that to my commit and pushed.

@benlesh
Copy link
Member

benlesh commented Jan 11, 2022

@jennings So sorry this is such a late response. I don't have a way to force the checks to rerun without getting into your branch and pushing up an empty commit or something. Github isn't letting me approve the checks to be run. If you can, please rebase and push again so I can approve the checks to be run. Otherwise, I guess we can open a new PR with this work.

This overload alleviates the observableFactory from needing to cast its
parameter to a more specific type.
@jennings jennings force-pushed the add-resource-to-using-type-signature branch from 296ec78 to 6ef371d Compare January 17, 2022 17:43
@jennings
Copy link
Author

jennings commented Jan 17, 2022

If you can, please rebase and push again so I can approve the checks to be run.

@benlesh Done.

@sgoll
Copy link

sgoll commented Aug 25, 2022

Quoting @cartant from #6497 (comment):

I suspect that using received close to no attention because it's not something that's used (pardon the pun) much.

I am one of the few users that use using from time to time and I have stumbled upon this problem several times. I'd love to see @jennings' changes implemented 🙂

@bman654
Copy link

bman654 commented Jul 26, 2023

I've encountered this as well. My workaround is to add this typings file to my project

// src/@types/rxjs-using.d.ts

import type { Observable, ObservableInput, ObservedValueOf, Unsubscribable } from "rxjs"

declare module "rxjs" {
  // fix type declaration of using.
  export function using<T extends ObservableInput<any>, R extends Unsubscribable | void>(
    resourceFactory: () => R,
    observableFactory: (resource: R) => T | void
  ): Observable<ObservedValueOf<T>>
}

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