Add internal API to collect platform version info #2368

hsubox76 · 2019-11-18T18:49:11Z

Adds an internal VersionService component that each package can create and register to report its own version.

Adds an internal PlatformLoggerService component initialized by the app package, with one method, getPlatformInfoString(), which collects version info from all version services and returns a string formatted according to Firebase's cross-platform X-firebase-client header standards. This string can then be sent to the backend as appropriate (to be implemented in future PRs).

mmermerkaya

Just a few style comments. Also left some comments on the design doc.

mmermerkaya · 2019-11-19T13:28:12Z

packages/app/src/platformLoggerService.ts

+    // version components.
+    return providers
+      .map(provider => {
+        const service = provider.getImmediate();


const service = provider.getImmediate() as VersionService; and then you can remove the other casts.

mmermerkaya · 2019-11-19T13:28:31Z

packages/app/src/platformLoggerService.ts

+        const component = provider.getComponent();
+        if (service && component && component.type === ComponentType.VERSION) {
+          const platformString =
+            platformLogString[(service as VersionService).library] ||


?? instead of || 🙂

Waiting for parent branch to merge so I can rebase and upgrade Typescript version to 3.7.

mmermerkaya · 2019-11-19T13:29:17Z

packages/app/src/platformLoggerService.ts

+      .map(provider => {
+        const service = provider.getImmediate();
+        const component = provider.getComponent();
+        if (service && component && component.type === ComponentType.VERSION) {


service && component?.type === ComponentType.VERSION

mmermerkaya · 2019-11-19T13:30:52Z

packages/app/src/platformLoggerService.ts

+import { platformLogString } from './constants';
+
+export class PlatformLoggerService {
+  constructor(private container: ComponentContainer) {}


readonly?

mmermerkaya · 2019-11-19T13:31:39Z

packages/app/src/constants.ts

@@ -16,3 +16,17 @@
 */

 export const DEFAULT_ENTRY_NAME = '[DEFAULT]';
+
+export const platformLogString: { [key: string]: string } = {


Is it necessary to widen the type? Do we expect this const to be extended or modified?

Also, CONST_CASE? 🙂

mmermerkaya · 2019-11-19T13:44:09Z

packages/app/test/platformLogger.test.ts

+  }
+}
+
+describe('Platform Logger Service Unit Tests', () => {


You can nest the two describe blocks in one main describe block.

describe('Platform Logger Service', () => { describe('Unit Tests', () => { // ... } describe('Integration Tests', () => { // ... } }

mmermerkaya · 2019-11-19T14:00:16Z

packages/app/src/platformLoggerService.ts

+          return null;
+        }
+      })
+      .filter((logString: string | null) => logString)


TS should be able to infer this type.

Type removed.

mmermerkaya · 2019-11-19T14:10:45Z

packages/app/src/constants.ts

+  'remote-config': 'fire-rc',
+  'storage': 'fire-gcs',
+  'firestore': 'fire-fst'
+};


} as const; to make them readonly.

mmermerkaya · 2019-11-19T14:19:00Z

packages/app/src/firebaseNamespaceCore.ts

+    return registerComponent(
+      new Component(
+        `${library}-version` as Name,
+        () => {


() => new VersionService(library, version),

mmermerkaya · 2019-11-22T15:58:53Z

packages/app/src/firebaseNamespaceCore.ts

+    }
+    registerComponent(
+      new Component(
+        `${library}-version` as Name,


I'm not a fan of this cast TBH, since it defeats the purpose of the Name type. Maybe we shouldn't consider versions actual Components and instead create a different type for them?

I think that it might be a good idea in the long term to have separate Map in ComponentContainer in addition to the Providers Map, and this new Map just stores key/value pairs. Can be used for versions now, and other possible key/value pair data going forward. But that requires a bit of modification to ComponentContainer that might not be a good idea right now as we're planning to do our first release with the migration to the component model next week right before a holiday. Since the actual registerVersion API wouldn't change we can change the implementation later when the component model is in use and stable.

Sure, sounds good.

mmermerkaya · 2019-11-27T15:15:54Z

packages/app/src/firebaseApp.ts

+    this._addComponent(
+      new Component(
+        'platform-logger',
+        (container: ComponentContainer) => new PlatformLoggerService(container),


TS should be able to infer this type.

mmermerkaya · 2019-11-27T15:22:38Z

packages/app/src/platformLoggerService.ts

+          // a good whitelist system.
+          const platformString =
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            (PLATFORM_LOG_STRING as any)[service.library] ?? service.library;


Instead of as any you can do this: PLATFORM_LOG_STRING[service.library as keyof typeof PLATFORM_LOG_STRING]. It's a bit longer but you wouldn't have to enable any.

Or in VersionService you can set the type of library to that, but that prevents setting arbitrary library versions and I think that was one of the requirements.

mmermerkaya · 2019-11-27T15:28:02Z

packages/messaging/src/controllers/window-controller.ts

@@ -318,7 +318,6 @@ export class WindowController extends BaseController {
              message_time: data[FN_CAMPAIGN_TIME],
              message_device_time: Math.floor(Date.now() / 1000)
            }
-            /* eslint-enable camelcase */


But why? :(

Argh, that's a bad merge, I keep putting it back in after merges/rebases but it keeps removing it again. I'll put it back.

Feiyang1 · 2019-11-27T22:25:54Z

packages/app/src/firebaseApp.ts

@@ -60,6 +64,28 @@ export class FirebaseAppImpl implements FirebaseApp {

    // add itself to container
    this._addComponent(new Component('app', () => this, ComponentType.PUBLIC));
+    this._addComponent(


The component registrations should be moved to index files at the top level, e.g. app/index.ts, app/index.node.ts and etc.

The reason FirebaseApp registration has to happen here is because the container lives within an app instance, but I plan to move the container outside, so FirebaseApp can register just like the other components.

Made a registerPlatformLogger function for the shared registration code between these files.

Feiyang1 · 2019-11-27T23:45:03Z

packages/app/src/firebaseNamespaceCore.ts

@@ -234,6 +236,22 @@ export function createFirebaseNamespaceCore(
      : null;
  }

+  function registerVersion(library: string, version: string): void {
+    if (library.match(/\s|\//)) {


Should we check version too? I'd also change the warning message to include version and to make it clear it is registering a version.

Should be a pretty robust warning message now!

Feiyang1 · 2019-11-28T00:14:13Z

packages/app/src/platformLoggerService.ts

+    // version components.
+    return providers
+      .map(provider => {
+        const service = provider.getImmediate() as VersionService;


provider.getImmediate() will throw in case of placeholder providers. You can call it with optional flag to make getImmediate() return null instead of throwing. Then check if service is null.

const service = provider.getImmediate({optional: true}) as VersionService;

We can also remove the type cast by using a custom type guard, but it needs a hack:

provider => { if (isVersionServiceProvider(provider)) { const service = provider.getImmediate(); // TODO: We can use this check to whitelist strings when/if we set up // a good whitelist system. const platformString = PLATFORM_LOG_STRING[ service.library as keyof typeof PLATFORM_LOG_STRING ] ?? service.library; return `${platformString}/${service.version}`; } else { return null; } } /** * * @param provider check if this provider provides a VersionService * * NOTE: Using Provider<'app-version'> is a hack to indicate that the provider provides VersionService * The provider is not necessarily a 'app-version' provider. */ function isVersionServiceProvider(provider: Provider<Name>): provider is Provider<'app-version'> { const component = provider.getComponent(); return component?.type === ComponentType.VERSION; }

Feiyang1 · 2019-11-28T00:17:16Z

packages/app/src/version-service.ts

+ * limitations under the License.
+ */
+
+export class VersionService {


Feel like we can just use a plain Object instead of a class to save an additional file?

TBH classes have very little overhead, but if we're going to use a plain object, could we also export and use a function like this:

function makeVersionService(library: string, service: string) { return {library, service}; }

It would make things more readable and would be useful in case we want to change the fields later or something?

We also type this object with an interface, so it's still type safe.

I remember classes gave us treeshaking problems before (#2006), so I'm a little cautious of using them, but the output you linked doesn't have Object.create which was the culprit. Maybe Object.create is being avoided now, or it is only emitted under certain condition?

Feiyang1 · 2019-11-28T00:21:47Z

packages/functions/src/config.ts

@@ -48,4 +50,5 @@ export function registerFunctions(instance: _FirebaseNamespace): void {
      .setServiceProps(namespaceExports)
      .setMultipleInstances(true)
  );
+  instance.registerVersion('functions', version);


Are you planning to add this to other SDKs in a separate PR?

Yes, planning to do a separate PR for every approver group.

Feiyang1

LGTM. Just a couple of nits.

Feiyang1 · 2019-12-03T18:50:25Z

packages/app/src/registerPlatformLogger.ts

+    )
+  );
+  // Register `app` package.
+  firebase.registerVersion(name, version);


Can we rename registerPlatformLogger to registerComponentsAndVersions() or something else that's not specific to platform logger?

Changed to registerCoreComponents

Feiyang1 · 2019-12-03T19:00:40Z

packages/app/src/platformLoggerService.ts

+}
+function isVersionServiceProvider(
+  provider: Provider<Name>
+): provider is Provider<'app-version'> {


Can you please add a comment that Provider<'app-version'> is a hack to indicate that it's a version component?

hsubox76 requested review from bojeil-google, Feiyang1, hiranya911 and wti806 as code owners November 18, 2019 18:49

hsubox76 requested review from mmermerkaya and removed request for bojeil-google and wti806 November 18, 2019 22:38

mmermerkaya reviewed Nov 19, 2019

View reviewed changes

hsubox76 force-pushed the ch-platform branch from 7853fd0 to 1b8607d Compare November 21, 2019 22:18

mmermerkaya reviewed Nov 22, 2019

View reviewed changes

Feiyang1 force-pushed the fei-component-integration2 branch from 6caf8ef to ae956b1 Compare November 22, 2019 19:06

hsubox76 requested review from alikn, andirayo, davideast, dwoffinden, erikeldridge, jamesdaniels, jsdt, mikelehen, ryanpbrewster, schmidt-sebastian, scottcrossen, yuchenshi and zijianjoy as code owners November 26, 2019 01:48

hsubox76 force-pushed the ch-platform branch from b339263 to d527708 Compare November 26, 2019 20:13

hsubox76 changed the base branch from fei-component-integration2 to master November 27, 2019 00:18

hsubox76 force-pushed the ch-platform branch 2 times, most recently from dcab55e to 5cb2514 Compare November 27, 2019 00:51

Add platform version logging functionality.

f085e2a

hsubox76 force-pushed the ch-platform branch from 5cb2514 to f085e2a Compare November 27, 2019 00:54

hsubox76 requested review from mmermerkaya and removed request for alikn, andirayo, davideast, erikeldridge, jamesdaniels, jsdt, mikelehen, ryanpbrewster, schmidt-sebastian, scottcrossen, yuchenshi and zijianjoy November 27, 2019 01:29

mmermerkaya approved these changes Nov 27, 2019

View reviewed changes

hsubox76 added 4 commits November 27, 2019 10:36

Address PR comments

8564b4b

[AUTOMATED]: Prettier Code Styling

611f370

Validate version string

6dc72df

Fix import

b912b38

Feiyang1 suggested changes Nov 28, 2019

View reviewed changes

hsubox76 added 4 commits December 2, 2019 12:59

Address PR comments

69c199b

[AUTOMATED]: Prettier Code Styling

3058b9b

Get package names from package.json and add tests

e2583e9

[AUTOMATED]: Prettier Code Styling

68808af

Feiyang1 approved these changes Dec 3, 2019

View reviewed changes

Address PR comments

d13746d

hsubox76 merged commit fe11de0 into master Dec 3, 2019

jamesdaniels mentioned this pull request Dec 4, 2019

feat(core): Register AngularFire and Angular versions with the JS SDK, if API available angular/angularfire#2258

Merged

hsubox76 added this to the next milestone Dec 10, 2019

firebase locked and limited conversation to collaborators Jan 3, 2020

hsubox76 deleted the ch-platform branch September 15, 2020 16:44

Add internal API to collect platform version info #2368

Add internal API to collect platform version info #2368

Uh oh!

Conversation

hsubox76 commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmermerkaya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsubox76 commented Nov 18, 2019 •

edited

Loading