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

TypeScript: Pluck has a type safety detonator in it #5188

Closed
Lonelind opened this issue Dec 20, 2019 · 3 comments
Closed

TypeScript: Pluck has a type safety detonator in it #5188

Lonelind opened this issue Dec 20, 2019 · 3 comments
Labels
bug Confirmed bug TS Issues and PRs related purely to TypeScript issues

Comments

@Lonelind
Copy link
Contributor

Lonelind commented Dec 20, 2019

Bug Report

Current Behavior with reproduction example
The pluck operator is working poorly with some cases in terms of type safety.

We use a rig to generate props for React components from Rx Observables. The function uses Partial of the Component Props type and expect this type to be returned. In general we have something like this:

interface MyPropType = {
    id: number;
}
interface ComponentProps = {
    myProp: MyPropType;
}

const Component: React.FC<ComponentProps> = ({ myProp }) => (
    <div>{myProp.id}</div>
);

// It's the general idea, not the actual implementation
const Container = withRx(Component)(({ myStream$ }) => ({
    myProp: myStream$.pipe(pluck('someField')),
}));

So, it's pretty fine with that in terms of type safety. Let's assume we have the following in myStream$:

interface MyData = {
    anyField: { id: number };
}

const myStream$: Observable<MyData> = /* ... */

Type inference for pluck will work in an unexpected way. If you look carefully, you'll see that if someField is absent from the expected object type than keyof can't be inferred and we have a general overload that gets some R from what it is expected to be:

// pluck.d.ts
/* ... */
export declare function pluck<T, R>(...properties: string[]): OperatorFunction<T, R>;

The problem in this case is that we have a type for myProp field and the R is inferred from it. So, we have a false expectation that there should be MyPropType returned from pluck. But suddenly, there is an undefined there, in runtime.

Expected behavior
It's expected to have any kind of type check error if we pass a key that's absent in the object type we have in the pluck input regardless of the expectations.

Environment

  • TypeScript version: 3.5.2
  • RxJS version: 6.5.2
@cartant
Copy link
Collaborator

cartant commented Dec 20, 2019

Yeah, this could be better. It's inferred as unknown:

const source = of({ foo: "bar" });
const plucked = source.pipe(pluck("goo")); // Observable<unknown>

Because, as you've pointed out, it matches the catch-all, rest parameter signature - but it should effect a compile-time error. The catch-all should include a rest parameter after the other parameters - as in this pipe signature.

@cartant cartant added bug Confirmed bug TS Issues and PRs related purely to TypeScript issues labels Dec 20, 2019
@Lonelind
Copy link
Contributor Author

Lonelind commented Dec 20, 2019

And if you are more specific, it has specific type, not the unknown.

interface A {
    foo: string;
}

const source: Observable<A> = of({ foo: 'bar' });
const plucked: Observable<A['foo']> = source.pipe(pluck('goo'));

This pluck thinks it returns string without any type errors. Keeping in mind that we can't be less specific in interfaces as we can with const/let, this can explode in an unexpected way. Say, somebody renamed property of the interface everywhere but forgot one single pluck and nothing warned them that there is a problem. And later — BOOM.

@Lonelind
Copy link
Contributor Author

Looks like this signature works for the catch-all and it definitely returns unknown that you should process manually:

export declare function pluck<T, K1 extends keyof T, K2 extends keyof T[K1], K3 extends keyof T[K1][K2], K4 extends keyof T[K1][K2][K3], K5 extends keyof T[K1][K2][K3][K4], K6 extends keyof T[K1][K2][K3][K4][K5]>(k1: K1, k2: K2, k3: K3, k4: K4, k5: K5, k6: K6, ...properties: string[]): OperatorFunction<T, unknown>;

benlesh pushed a commit that referenced this issue Jan 20, 2020
…5192)

* fix(pluck): fix pluck's catch-all signature for better type safety

Change pluck's catch-all signature that is causing troubles when the
passed string is not a key in the argument object.

Issue: #5188

* fix(pluck): fix signature and tests

Correct the signtaure to be accepted by tests and to return unknown in
general

* fix(pluck): add test case to check type inference
martinsik pushed a commit to martinsik/rxjs that referenced this issue Feb 15, 2020
…eactiveX#5192)

* fix(pluck): fix pluck's catch-all signature for better type safety

Change pluck's catch-all signature that is causing troubles when the
passed string is not a key in the argument object.

Issue: ReactiveX#5188

* fix(pluck): fix signature and tests

Correct the signtaure to be accepted by tests and to return unknown in
general

* fix(pluck): add test case to check type inference
@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Confirmed bug TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

2 participants