User agent client hints support #18
Conversation
src/extra/AddonInfo.ts
Outdated
| } | ||
|
|
||
| export class AddonInfo implements WebTelemetryAddon<AddonInfoData & AddonInfoMetadata, {}> { | ||
| data(): AddonInfoData & AddonInfoMetadata { |
There was a problem hiding this comment.
Паша data синхронный метод.
А getHighEntropyValues это ассинхронная операция.
Т.е. у тебя возможен вариант когда не будет нужных данных при чтении из объекта который вернул data() .
Тут по хорошему у тебя должны были сломаться какие-то тесты. Глянь есть ли такие тесты, если нет напиши.
src/extra/AddonInfo.ts
Outdated
|
|
||
| return highEntropyValues; | ||
| } | ||
|
|
||
| metadata() { | ||
| return {}; |
There was a problem hiding this comment.
Новые данные должный сюда идти, а не в data.
Все что приходит в данные ложится в именные столбцы в таблицу.
Все, что приходит в metadata ложиться в поле metadata в виде json.
Т.е. новые поля без миграции таблиц мы добавляем в metadata
There was a problem hiding this comment.
Паша, просто завернуть всё в await — не вариант.
Во-первых, могут возникнуть ситуации, когда код вместо параллельного выполнения будет выполняться последовательно.
Во-вторых, теперь придётся писать await везде, включая сторонние аддоны.
Мы с тобой обсуждали другой подход.
Давай еще раз обсудим голосом.
src/WebTelemetryBase.spec.ts
Outdated
| @@ -3,21 +3,21 @@ import { WebTelemetryBase } from './WebTelemetryBase'; | |||
|
|
|||
| class Addon1 implements WebTelemetryAddon<{ addon1Data: string }, { addon1Metadata: string }> { | |||
| data() { | |||
| return { addon1Data: 'addon1Data' }; | |||
| return Promise.resolve({ addon1Data: 'addon1Data' }); | |||
There was a problem hiding this comment.
Не нужно. Обарачиваем в Promise.resolve на староне WebTemetryBase.
src/WebTelemetryBase.spec.ts
Outdated
| } | ||
| } | ||
|
|
||
| class Addon2 implements WebTelemetryAddon<{ addon2Data: string }, { addon2Metadata: string }> { | ||
| data() { | ||
| return { addon2Data: 'addon2Data' }; | ||
| return Promise.resolve({ addon2Data: 'addon2Data' }); |
src/WebTelemetryBase.spec.ts
Outdated
| } | ||
|
|
||
| metadata() { | ||
| return { addon2Metadata: 'addon2Metadata' }; | ||
| return Promise.resolve({ addon2Metadata: 'addon2Metadata' }); |
There was a problem hiding this comment.
Не нужно Promise.resolve . Просто оборачивай вызов metadata в Promise.resolve, а не даныне в metadata.
src/WebTelemetryBase.spec.ts
Outdated
| @@ -26,12 +26,12 @@ class WebTelemetry<T> extends WebTelemetryBase<T, T> { | |||
| return payload; | |||
| } | |||
|
|
|||
| protected scheduleSend() { | |||
| protected async scheduleSend() { | |||
| @@ -62,7 +62,8 @@ describe('WebTelemetryBase', () => { | |||
| addon2Metadata: 'addon2Metadata', | |||
| }; | |||
|
|
|||
| instance.push({ data: 'data' }, { metadata: 'metadata' }); | |||
| await instance.push({ data: 'data' }, { metadata: 'metadata' }); | |||
There was a problem hiding this comment.
push у нас не ассинхронный.
There was a problem hiding this comment.
если убрать await, то этот тест не сможет правильно отработать, так как getEvents()[0] вернёт undefined. Такой же тест находится на webtelemetrykv.spec.ts, на 16й строке, старался на него ориентироваться
src/extra/KVDataFrameTime.spec.ts
Outdated
| @@ -18,7 +18,7 @@ const fakeTransport = new (class implements WebTelemetryTransport { | |||
| } | |||
|
|
|||
| async subscribe() { | |||
| return this.subscriber; | |||
| return await this.subscriber; | |||
src/extra/AddonNavigationPerf.ts
Outdated
| @@ -71,11 +71,11 @@ export class AddonNavigationPerf implements WebTelemetryAddon<AddonNavigationPer | |||
| } | |||
| } | |||
|
|
|||
| data() { | |||
| async data() { | |||
src/extra/AddonNavigationPerf.ts
Outdated
| return { ...this.value }; | ||
| } | ||
|
|
||
| metadata() { | ||
| async metadata() { |
| } | ||
|
|
||
| metadata() { | ||
| return {}; | ||
| private async getHighEntropyValues(): Promise<UserAgentDataValues | null> { |
src/extra/AddonFCP.ts
Outdated
| return { | ||
| FCP: Math.round(this.value), | ||
| }; | ||
| } | ||
|
|
||
| metadata() { | ||
| async metadata() { |
📦 Published PR as canary version:
2.0.3--canary.18.12864022629.0✨ Test out this PR locally via: