Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

@novocaine
Copy link
Contributor

@novocaine novocaine commented Nov 23, 2021

Integrate Error and Screen event types from https://github.com/matrix-org/matrix-analytics-events


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://61ade273238b958da8153a92--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@novocaine novocaine marked this pull request as ready for review November 30, 2021 00:59
@novocaine novocaine requested a review from a team as a code owner November 30, 2021 00:59
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm - thanks!

package.json Outdated
"katex": "^0.12.0",
"linkifyjs": "^2.1.9",
"lodash": "^4.17.20",
"matrix-analytics-events": "https://github.com/matrix-org/matrix-analytics-events.git",
Copy link
Member

Choose a reason for hiding this comment

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

preferably this ends up on npm or at least tied to a commit, to avoid cases where we're debugging different checkouts and versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we depend on the js-sdk? I think it's kind of a similar situation..

Copy link
Contributor

Choose a reason for hiding this comment

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

During development the package.json points to github:matrix-org/matrix-js-sdk#develop. However the release script will replace that value with a version deployed to npm during the release cycle.

This happens in element-web/release.sh or matrix-react-sdk/release.sh for example

Copy link
Member

Choose a reason for hiding this comment

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

also, when it's in develop we have the checkout linked anyways so it doesn't really affect much. The widget-api though doesn't traditionally get linked in dev, so we use an npm release. For the linter stuff, we point at a commit for reasons I cannot recall.

if (this.accountPasswordTimer !== null) clearTimeout(this.accountPasswordTimer);
}

trackScreenChange(durationMs: number): void {
Copy link
Member

Choose a reason for hiding this comment

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

explicit member access please

foo: string;
};
export interface ITestEvent extends IEvent {
eventName: "jest_test_event";
Copy link
Member

Choose a reason for hiding this comment

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

We should follow our own docs in tests too 😇

Suggested change
eventName: "jest_test_event";
eventName: "JestTestEvent";

novocaine and others added 4 commits December 6, 2021 11:44
This is fine for now; when we need an updated version of the stubs to raise new events we can just bump the githash to a newer version
@novocaine novocaine merged commit 43f264c into develop Dec 6, 2021
@novocaine novocaine deleted the integrate-analytics-stubs branch December 6, 2021 10:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants