feat(pubsub): Typescript improvements and cleanup #10954

stocaaro · 2023-02-09T01:23:57Z

Description of changes

Changes made:

Use typescript-coverage-report output to identify and fix ts issues
- TS coverage is now 94.83% and gated at regression below 93%
Re-add coverage protection for the build and update to omit paho.js
Removed the unused code in AWSAppSyncProvider.ts
Removed paho as a dependency (we're using the vendor lib version right now).

TS Coverage:

Before ~88% (ignoring Paho), shows 79.9% with Paho counted
After 94.83%

Description of how you validated changes

Fixed unit testing issues
Manually tested against IoT/API/Datastore

Checklist

PR description included
yarn test passes
Tests are changed or added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

stocaaro · 2023-02-09T16:44:43Z

packages/pubsub/__tests__/PubSub-unit-test.ts

@@ -440,7 +448,7 @@ describe('PubSub', () => {
 				provider: 'MqttOverWSProvider',
 			});

-			expect(mqttOverWSProvider.isSSLEnabled).toBe(false);
+			expect(mqttOverWSProvider['isSSLEnabled']).toBe(false);


Change because it is accessing a protected getter

stocaaro · 2023-02-09T16:46:44Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

-		return !this.options
-			.aws_appsync_dangerously_connect_to_http_endpoint_for_testing;
+		return !this.options[
+			'aws_appsync_dangerously_connect_to_http_endpoint_for_testing'


Changing access pattern since we opted not to have this member explicitly on the options type.

I made a run at rewriting the ProviderOptions interface as a Record type. Turned into cascading problems that couldn't be teased out in the 20 minutes I gave it. I'm sure it can be done, but since this is a key customer facing interface, I would prefer to leave it or wrap it in more testing before retooling as a type.

In this case, we have added all other variables as fields on the interface. We opted not to add this one as the name isn't something we want folks autocompleting to / seeing in the public surface. Which is why TS prefers not to directly access it.

This is used also for mock use case IIRC. That is a CLI feature that allows you to test the API without doing amplify push

I think it makes sense to not expose that unless is generated automatically by CLI

stocaaro · 2023-02-09T16:50:43Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

 		logger.debug({ err });
-		const message = err['message'] ?? '';
+		const message = String(err.message ?? '');


Errors are a pain in the current version of ts. Newer versions default to unknown and for narrowing. Currently catches will be any which makes coverage reporting unhappy. Will look at upgrading

stocaaro · 2023-02-09T16:53:08Z

packages/pubsub/src/Providers/MqttOverWSProvider.ts

+	aws_pubsub_endpoint?: string;
+}
+
+interface PahoClient {


This is sort of like inlining a d.ts file for Paho.Client so that we aren't interacting with it as any.

Excellent call. Pulling it in.

Yes I would use that

codecov-commenter · 2023-02-09T17:13:23Z

Codecov Report

Merging #10954 (035cdb0) into main (91e0c8f) will increase coverage by 0.31%.
The diff coverage is 90.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #10954      +/-   ##
==========================================
+ Coverage   81.90%   82.21%   +0.31%     
==========================================
  Files         194      193       -1     
  Lines       19376    19295      -81     
  Branches     4179     4165      -14     
==========================================
- Hits        15870    15864       -6     
+ Misses       3217     3143      -74     
+ Partials      289      288       -1

Impacted Files	Coverage Δ
packages/core/src/constants.ts	`100.00% <ø> (ø)`
packages/core/src/index.ts	`100.00% <ø> (ø)`
packages/pubsub/src/Providers/index.ts	`100.00% <ø> (ø)`
packages/pubsub/src/index.ts	`100.00% <ø> (ø)`
packages/pubsub/src/types/PubSub.ts	`100.00% <ø> (ø)`
...ackages/pubsub/src/utils/ConnectionStateMonitor.ts	`100.00% <ø> (ø)`
packages/pubsub/src/PubSub.ts	`91.17% <75.00%> (-1.25%)`	⬇️
...ackages/pubsub/src/Providers/MqttOverWSProvider.ts	`92.22% <90.32%> (+0.69%)`	⬆️
.../src/Providers/AWSAppSyncRealTimeProvider/index.ts	`95.68% <91.66%> (-0.39%)`	⬇️
packages/pubsub/src/Providers/AWSIotProvider.ts	`100.00% <100.00%> (ø)`
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cshfang

I left some comments. There's nothing blocking but this is a Typescript improvement PR after all so I was being somewhat nit-picky. Some of it probably just stems from my unfamiliarity with pubsub so thank you for bearing with me.

cshfang · 2023-02-09T17:52:34Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

+		_topics: string[] | string,
+		_msg: object,
+		_options?: AWSAppSyncRealTimeProviderOptions


This isn't from this PR but why the underscores? 🤔

Can _msg be typed as a Record<K, V>?

I'm not sure if its idiomatic in JS/TS, but in other languages it is used to denote variables that are not used. In this case, I read it as, we take the standard inputs, but then no matter what raise, so these variables don't matter.

Ah. The pattern I see most commonly would be simply _, __ without naming the unused var. I find this _foo pattern confusing especially since we also use this convention to denote private vars/functions in classes.

Having said that, this might be another candidate for audit/align/cleanup for the team. We could consider turning on this lint rule: https://eslint.org/docs/latest/rules/no-unused-vars and defining a regex pattern for ignoring unused vars. Def not a blocker for this PR though

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

-		return !this.options
-			.aws_appsync_dangerously_connect_to_http_endpoint_for_testing;
+		return !this.options[
+			'aws_appsync_dangerously_connect_to_http_endpoint_for_testing'


packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

cshfang · 2023-02-09T19:43:40Z

packages/pubsub/src/Providers/AWSIotProvider.ts

@@ -15,8 +15,8 @@ export class AWSIoTProvider extends MqttOverWSProvider {
 		super(options);
 	}

-	protected get region() {
-		return this.options.aws_pubsub_region;
+	protected get region(): string | undefined {


This is a good example of when we would want undefined for sure

packages/pubsub/src/Providers/MqttOverWSProvider.ts

+	aws_pubsub_endpoint?: string;
+}
+
+interface PahoClient {


cshfang · 2023-02-09T19:52:35Z

packages/pubsub/src/Providers/MqttOverWSProvider.ts

 	}

-	protected get endpoint() {
+	protected get endpoint(): string | undefined | Promise<string> {


In what use case would this be a promise? This isn't a public API but it seems like if it's sometimes a Promise, it'd have to be awaited so why not just make it always a Promise?

This function is overridden in the IoT Provider, which returns a Promise<string>

packages/pubsub/src/PubSub.ts

cshfang

Have a couple lingering comments but I don't think they're blocking

cshfang · 2023-02-13T19:49:40Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

@@ -69,11 +73,29 @@ const customDomainPath = '/realtime';

 type GraphqlAuthModes = keyof typeof GRAPHQL_AUTH_MODE;

+type DataObject = {


packages/pubsub/src/Providers/MqttOverWSProvider.ts

 	}

-	protected get endpoint() {
+	protected get endpoint(): string | undefined | Promise<string> {


elorzafe

Thanks @stocaaro take a look on the comments we can chat more offline if you want

elorzafe · 2023-02-13T18:44:05Z

packages/pubsub/src/Providers/MqttOverWSProvider.ts

@@ -1,11 +1,12 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 import * as Paho from '../vendor/paho-mqtt';
+import { Client as PahoClient } from 'paho-mqtt';


Is this is a type only thing, should use the import from the type package?

Correct. Types only. Dev dependency. I originally had inlined this interface and adopted this package approach for typing paho at Jim and Chris's recommendations

Cool can we do import type to make it explicit, I got a little scared about that import statement

elorzafe · 2023-02-13T18:45:34Z

packages/pubsub/src/index.ts

@@ -5,7 +5,6 @@ export { PubSub } from './PubSub';
 export { CONNECTION_STATE_CHANGE } from './Providers/constants';
 export { ConnectionState, CONTROL_MSG } from './types';
 export {
-	AWSAppSyncProvider,


This might be consider a breaking change :/

elorzafe · 2023-02-13T18:47:32Z

packages/pubsub/src/types/Provider.ts

-	publish(topics: string[] | string, msg: any, options?: ProviderOptions): void;
+	publish(
+		topics: string[] | string,
+		msg: Record<string, unknown> | string,


This looks good!

elorzafe · 2023-02-13T18:55:15Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

 	Hub.dispatch('api', { event, data, message }, 'PubSub', AMPLIFY_SYMBOL);
 };

 export type ObserverQuery = {
 	observer: ZenObservable.SubscriptionObserver<any>;
 	query: string;
-	variables: object;
+	variables: Record<string, unknown>;


I would double check this with Data team

elorzafe · 2023-02-13T18:55:37Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

 export interface AWSAppSyncRealTimeProviderOptions extends ProviderOptions {
 	appSyncGraphqlEndpoint?: string;
 	authenticationType?: GraphqlAuthModes;
 	query?: string;
-	variables?: object;
+	variables?: Record<string, unknown>;


I would check this with Data team

Agreed that this needs review from the data team.

Regarding data team review, I've been watching the api-graphql category for issues. The subscription interface is translated to Observable on output with strongly typed inputs, so I don't expect this set of type changes to cause any impact to api/datastore users.

elorzafe · 2023-02-13T21:39:51Z

packages/pubsub/src/types/Provider.ts


 	subscribe(
 		topics: string[] | string,
 		options?: ProviderOptions
-	): Observable<any>;
+	): Observable<Record<string, unknown>>;


Is this always the case, can be returned something different? like a string?

In all cases, the observable emits objects of one type or another. I had originally normalized these to object which Chris had me refactor as Record<string, unknown> as this is more idiomatic with TS since it will encourage the user to clarify the unknown values received back.

packages/pubsub/tsconfig.build.json

elorzafe · 2023-02-13T21:44:17Z

packages/pubsub/src/PubSub.ts

-					error: error => observer.error({ provider, error }),
+					next: (value: Record<string, unknown>) =>
+						observer.next({ provider, value }),
+					error: (error: Record<string, unknown>) =>


I am not sure about this, maybe this to change as well

https://github.com/aws-amplify/amplify-js/blob/main/packages/pubsub/src/Providers/MqttOverWSProvider.ts#L213

You're right. Also, several of these comments circle around the same topic. I find myself convinced that:

next values can be strings as well as Records/objects

The category subscription returns {provider: x, value: y} instead of the string/Record itself, so that can be more clearly typed.

The publish and subscribe value types should be represented by a shared type instead of being repeated everywhere.

elorzafe · 2023-02-13T21:45:26Z

packages/pubsub/src/Providers/MqttOverWSProvider.ts

@@ -1,11 +1,12 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 import * as Paho from '../vendor/paho-mqtt';
+import { Client as PahoClient } from 'paho-mqtt';
 import { v4 as uuid } from 'uuid';
 import Observable, { ZenObservable } from 'zen-observable-ts';


Can we get rid of this zen-observable-ts dependency?

I remember having bundle size issues with this

Took a look at removing zen-observable-ts, which should be replaceable with @types/zen-observable. It didn't go well. VSCode is happy, but the build fails with errors like zen observable _1.default is not a constructor. There's got to be a work around, but I didn't find it in my 30 minute time box, so I'm not including any related changes in this update.

elorzafe

I only have minor non blocking comments, I am good to merge this after checking usage numbers of the old AppSync provider

elorzafe · 2023-02-15T00:24:28Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

-		return !this.options
-			.aws_appsync_dangerously_connect_to_http_endpoint_for_testing;
+		return !this.options[
+			'aws_appsync_dangerously_connect_to_http_endpoint_for_testing'


This is used also for mock use case IIRC. That is a CLI feature that allows you to test the API without doing amplify push

I think it makes sense to not expose that unless is generated automatically by CLI

elorzafe · 2023-02-15T00:25:02Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

@@ -270,7 +303,7 @@ export class AWSAppSyncRealTimeProvider extends AbstractPubSubProvider {
 		subscriptionId,
 	}: {
 		options: AWSAppSyncRealTimeProviderOptions;
-		observer: ZenObservable.SubscriptionObserver<any>;
+		observer: PubSubContentObserver;


elorzafe · 2023-02-15T00:25:46Z

packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider/index.ts

-	private _logStartSubscriptionError(subscriptionId, observer, err) {
+	private _logStartSubscriptionError(
+		subscriptionId: string,
+		observer: ZenObservable.SubscriptionObserver<any>,


Can this also use an internal type?

Yep. Good call.

elorzafe · 2023-02-15T00:30:07Z

packages/pubsub/src/Providers/MqttOverWSProvider.ts

@@ -1,11 +1,12 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 import * as Paho from '../vendor/paho-mqtt';
+import { Client as PahoClient } from 'paho-mqtt';


Cool can we do import type to make it explicit, I got a little scared about that import statement

elorzafe · 2023-02-15T00:33:52Z

packages/pubsub/src/types/PubSub.ts

@@ -1,5 +1,6 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
+import { ZenObservable } from 'zen-observable-ts';


Can this be imported as type, or import SubscriptionObserver?

SubscriptionObserver isn't exported directly.

The d.ts file looks like this:

export declare namespace ZenObservable { interface SubscriptionObserver<T> { closed: boolean; next(value: T): void; error(errorValue: any): void; complete(): void; } ... }

Am I misunderstanding what you're asking for here? What would I import to do this?

I think Francisco is referring to: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export

Have discussed with Francisco. We'll be looking at retooling our zen-observable usage for v6. I did some exploration here to try to excise zen-observable-ts, however the @types/zen-observable package is missing some of the features zen-observable patches onto global. Would be nice to have a ts first observable lib that is bundle-size efficient.

stocaaro · 2023-03-01T18:54:12Z

On request, I have run the integ tests against this change, which prompted working out some Angular build issues.

svidgen

It's hard to validate with my eyeballs, but from a DataStore/API perspective, it looks like most of the changes here should be non-impactful. If we've got passing integ tests, I think that's good for me.

WolfWalter · 2023-03-14T11:53:03Z

This PR changed the PubSub message format in an unexpected way:
5147c5c#diff-2a29f8a41f2b66733bb64a134e53acb5e67615e026667885a5686ce933d3869eL267-R300

topicSymbol is now an object key and the Symbol is not used anymore. This totally changed how to get the topic and the message object gets altered in an unexpected way.

Was that really intentional @stocaaro and @elorzafe?

stocaaro added 2 commits February 8, 2023 18:56

feat(pubsub): Typescript improvements and cleanup

ef594df

fix: Finish removing the old AppSync provider

eb75633

stocaaro requested review from a team as code owners February 9, 2023 01:23

stocaaro and others added 3 commits February 9, 2023 10:35

fix: Providername unpacking test issue

5efe137

Merge branch 'main' into pubsub-ts-improvements

7aa9d96

fix: Remove unnecessary optional

b2a08cc

stocaaro commented Feb 9, 2023

View reviewed changes

cshfang reviewed Feb 9, 2023

View reviewed changes

stocaaro and others added 2 commits February 10, 2023 15:49

fix: Remove any/object and other PR comments

ef28386

Merge branch 'main' into pubsub-ts-improvements

79e6109

stocaaro requested a review from cshfang February 10, 2023 22:51

cshfang previously approved these changes Feb 13, 2023

View reviewed changes

elorzafe reviewed Feb 13, 2023

View reviewed changes

fix: Type improvements

ddd09bf

stocaaro dismissed cshfang’s stale review via ddd09bf February 14, 2023 15:34

stocaaro added 2 commits February 14, 2023 10:26

Content / Observer typing refactor

40e4e13

Content / Observer typing refactor

1a9d9f7

stocaaro requested review from cshfang and elorzafe February 14, 2023 21:09

elorzafe previously approved these changes Feb 15, 2023

View reviewed changes

stocaaro and others added 2 commits February 15, 2023 09:57

Merge branch 'main' into pubsub-ts-improvements

4e80c2b

fix: Type update from comment

45d500e

stocaaro dismissed elorzafe’s stale review via 45d500e February 15, 2023 16:10

Merge branch 'main' into pubsub-ts-improvements

187bba4

stocaaro requested a review from elorzafe February 16, 2023 20:23

stocaaro added 3 commits February 17, 2023 09:41

Merge branch 'main' into pubsub-ts-improvements

45cb049

Merge branch 'main' into pubsub-ts-improvements

31b3264

Merge branch 'main' into pubsub-ts-improvements

a3cabc2

stocaaro and others added 4 commits February 24, 2023 12:28

Merge branch 'main' into pubsub-ts-improvements

e52ce3c

fix: Reintroduce paho to fix angular build issues

bd502d7

fix: Revert to including no paho dependencies and having custom typing

994b0f2

Merge branch 'main' into pubsub-ts-improvements

fecf1db

svidgen approved these changes Mar 1, 2023

View reviewed changes

elorzafe approved these changes Mar 1, 2023

View reviewed changes

Merge branch 'main' into pubsub-ts-improvements

035cdb0

stocaaro merged commit 5147c5c into aws-amplify:main Mar 1, 2023

WolfWalter mentioned this pull request Mar 22, 2023

PubSub 'topicSymbol' is now an entry in the message instead of a JS symbol #11114

Closed

3 tasks

renovate bot mentioned this pull request Sep 6, 2023

fix(deps): update dependency aws-amplify to v5 - autoclosed ciaran-finnegan/secret-sharer-server#91

Closed

1 task

		@@ -69,11 +73,29 @@ const customDomainPath = '/realtime';

		type GraphqlAuthModes = keyof typeof GRAPHQL_AUTH_MODE;

		type DataObject = {

feat(pubsub): Typescript improvements and cleanup #10954

feat(pubsub): Typescript improvements and cleanup #10954

Conversation

stocaaro commented Feb 9, 2023 • edited Loading

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 9, 2023 • edited Loading

Codecov Report

cshfang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

cshfang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

elorzafe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorzafe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stocaaro commented Mar 1, 2023

svidgen left a comment

Choose a reason for hiding this comment

WolfWalter commented Mar 14, 2023

stocaaro commented Feb 9, 2023 •

edited

Loading

codecov-commenter commented Feb 9, 2023 •

edited

Loading