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

fix(subscription) Updated request return type correctly match apollo-link #566

Closed

Conversation

wolfeidau
Copy link

Issue #, if available:

Description of changes:

Small change to specify the return type on a method implemented from ApolloLink.

Current type which doesn't compile with apollo-link 1.2.14.

    request(operation: Operation): Observable<unknown>;

New type after change which compiles

    request(operation: Operation): Observable<FetchResult> | null;

Compile error before change was as follows:

ERROR in releases-ui/node_modules/aws-appsync-subscription-link/lib/subscription-handshake-link.d.ts(18,5):
18:5 Property 'request' in type 'SubscriptionHandshakeLink' is not assignable to the same property in base type 'ApolloLink'.
  Type '(operation: Operation) => Observable<unknown>' is not assignable to type '(operation: Operation, forward?: NextLink | undefined) => Observable<FetchResult<{ [key: string]: any; }, Record<string, any>, Record<string, any>>> | null'.
    Type 'Observable<unknown>' is not assignable to type 'Observable<FetchResult<{ [key: string]: any; }, Record<string, any>, Record<string, any>>>'.
      Type 'unknown' is not assignable to type 'FetchResult<{ [key: string]: any; }, Record<string, any>, Record<string, any>>'.
        Type 'unknown' is not assignable to type 'ExecutionResult<{ [key: string]: any; }>'.
    16 |     private clientObservers;
    17 |     constructor(subsInfoContextKey: any);
  > 18 |     request(operation: Operation): Observable<unknown>;
       |     ^
    19 |     connectNewClients(connectionInfo: MqttConnectionInfo[], observer: ZenObservable.Observer<FetchResult>, operation: Operation): Promise<any[]>;
    20 |     connectNewClient(connectionInfo: MqttConnectionInfo, observer: ZenObservable.Observer<FetchResult>, selectionNames: string[]): Promise<any>;
    21 |     subscribeToTopics<T>(client: any, topics: string[], observer: ZenObservable.Observer<T>): Promise<unknown[]>;
Version: typescript 3.9.3

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

✅ 👍 💯

…link

Current type which doesn't compile.

```
    request(operation: Operation): Observable<unknown>;
```

New type after change which compiles

```
    request(operation: Operation): Observable<FetchResult> | null;
```
@bcdxn
Copy link

bcdxn commented Mar 25, 2022

I'm still seeing this issue in @apollo/client@3.5.10 and aws-appsync-subscription-link@3.0.10. Is there a workaround to make TypeScript happy until the PR is merged?

@@ -51,7 +51,7 @@ export class SubscriptionHandshakeLink extends ApolloLink {
this.subsInfoContextKey = subsInfoContextKey;
}

request(operation: Operation) {
request(operation: Operation): Observable<FetchResult> | null {
Copy link

Choose a reason for hiding this comment

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

hmm I dont if the typing is right here. Looking at this line, it might be observable of a different type too.

Would you be interested in taking a closer look at this?

Copy link

Choose a reason for hiding this comment

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

The linked line appears to be type Observable<unknown> since there is no next on the observable. I think we could cast the observable on line 71 to be Observable<FetchResult>.

@VRMink
Copy link

VRMink commented Jul 7, 2022

I am also seeing this issue
"@apollo/client": "^3.0.0",
"apollo-angular": "^3.0.1",
"aws-appsync-auth-link": "^3.0.7",
"aws-appsync-subscription-link": "^3.1.0",

@dpilch
Copy link

dpilch commented Jul 7, 2022

Moving to #727

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.

None yet

6 participants