Overhaul SDK metrics #5830
Overhaul SDK metrics #5830
Conversation
| @@ -25,28 +25,30 @@ import { readFileSync } from "node:fs"; | |||
|
|
|||
| type Fixture = "node" | "react-native" | "electron"; | |||
|
|
|||
| // TODO: Update this to use ESM friendly APIs | |||
| describe.skip("Analytics", () => { | |||
| describe("Analytics", () => { | |||
| function resolvePath(fixture: Fixture) { | |||
| return path.resolve(__dirname, "fixtures", fixture); | |||
| } | |||
|
|
|||
| function getRealmVersion() { | |||
There was a problem hiding this comment.
Seeing some slight differences in the getRealmVersion() used here in the test and the implementation in the script. (E.g. readJsonSync() vs JSON.parse() and readFileSync()). Should probably use the same implementation here as in the script.
There was a problem hiding this comment.
I agree, using the same implementation will make it easier to read and understand the code
| import commandLineArgs from "command-line-args"; | ||
| import fse from "fs-extra"; |
There was a problem hiding this comment.
fs-extra and command-line-args are both missing as dependencies.
There was a problem hiding this comment.
command-line-args is in the root package.json and I guess we should move it. And we don't see the issue with fs-extra in the integration tests as integration-tests/environments/electron/package.json has it as a dependency. I'll add both to the realm package's dependencies.
| import process from "node:process"; | ||
| import https from "node:https"; | ||
| import os from "node:os"; | ||
| import console from "node:console"; |
There was a problem hiding this comment.
I'm surprised this is needed 🤔 Is this due to an eslint rule or something?
There was a problem hiding this comment.
Yes, ESLint was loudly complaining.
| if (log) { | ||
| doLog = (msg) => console.log(msg); | ||
| } else { | ||
| // eslint-disable-next-line no-unused-vars | ||
| doLog = () => { | ||
| /* don't log */ | ||
| }; | ||
| } |
There was a problem hiding this comment.
In general, I don't really like this pattern of reassigning a function - as something might capture it in a scoped variable and it won't update as expected.
In general, I would rather that we implement a (const) log function that does a check on the options.log before calling console.log or similar.
Or even better, that we just rely on the "debug" package, which we already have a dependency on - that way we could get rid of the --log runtime parameter, leaving just --dry-run which could be replaced with the REALM_DISABLE_ANALYTICS (which is already essentially doing the same anyway) - and if we did that we could drop the command-line-args dependency entirely. Just a thought ...
There was a problem hiding this comment.
Good point. I'll rewrite it to use the debug package and see if I can get rid of command-line-args
| @@ -64,7 +81,10 @@ describe.skip("Analytics", () => { | |||
| expect(data.Version).equals("1.2.3"); | |||
| expect(data.Framework).equals("electron"); | |||
| expect(data["Framework Version"]).equals("1.0.1"); | |||
There was a problem hiding this comment.
This test seems to be failing. It is expecting ^1.0.0
| @@ -73,6 +93,9 @@ describe.skip("Analytics", () => { | |||
| expect(data.Version).equals("1.2.3"); | |||
| expect(data.Framework).equals("react-native"); | |||
| expect(data["Framework Version"]).equals("1.0.1"); | |||
There was a problem hiding this comment.
Same test from above is failing here as well
This reverts commit bf860e4.
What, How & Why?
This closes #5242
Adapting script and tests from #5807
I have reintroduced
dependencies.listand I am open to other ways to find the version of Realm Core.☑️ ToDos
Compatibilitylabel is updated or copied from previous entryCOMPATIBILITY.mdpackage.jsons (if updating internal packages)Breakinglabel has been applied or is not necessaryIf this PR adds or changes public API's: