Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to core's next-major #6416

Closed
wants to merge 22 commits into from
Closed

Upgrade to core's next-major #6416

wants to merge 22 commits into from

Conversation

kneth
Copy link
Contributor

@kneth kneth commented Jan 26, 2024

What, How & Why?

This closes # ???

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary

Copy link

coveralls-official bot commented Jan 26, 2024

Coverage Status

coverage: 85.663% (+0.3%) from 85.346%
when pulling 276dc67 on lj/core-next-major
into 23164b9 on main.

Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Here are some initial comments 🙂

Comment on lines 23 to 26
/**
* Log levels used by Realm
*
* Off Be silent.
* Fatal Be silent unless when an error is fatal.
* error Be silent unless when there is an error.
* warn Be silent unless when there is an error or a warning.
* Info Reveal information about what is going on, but in a
* minimalistic fashion to avoid general overhead from logging
* and to keep volume down.
* Detail Same as 'Info', but prioritize completeness over minimalism.
* Debug Reveal information that can aid debugging, no longer paying
* attention to efficiency.
* Trace A version of 'Debug' that allows for very high volume
* output.
* All Same as 'Trace' but with even more output.
*/
export enum NumericLogLevel {
Copy link
Contributor

Choose a reason for hiding this comment

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

For an improved DX, we can put the individual docs for each level immediately above each enum constant. See e.g. this enum.

packages/realm/src/Logger.ts Outdated Show resolved Hide resolved
Comment on lines 299 to 161
static setLogLevel(_arg: LogLevel | LogArgs) {
// It is not possible to overload a static function: https://github.com/microsoft/TypeScript/issues/18945
// FIXME: don't use `arguments` but find a proper type
if (arguments.length === 1) {
// eslint-disable-next-line prefer-rest-params
const arg = arguments[0];
if (arg.level) {
const level = arg.level;
if (arg.category) {
const category = arg.category;
assert(Object.values(LogCategory).includes(category));
binding.LogCategoryRef.getCategory(category).setDefaultLevelThreshold(toBindingLoggerLevel(level));
} else {
Object.values(LogCategory).forEach((category) => {
Realm.setLogLevel({ level, category });
});
}
} else {
const level = arg;
Object.values(LogCategory).forEach((category) => {
Realm.setLogLevel({ level, category });
});
}
} else {
throw new Error(`Wrong number of arguments - expected 1, got ${arguments.length}`);
}
}
Copy link
Contributor

@elle-j elle-j Jan 31, 2024

Choose a reason for hiding this comment

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

I think we can simplify this by just taking the category as an optional second arg. (Just an alternative to consider.)

I don't think it's necessary to iterate over the allowed categories and setting the log level on each category if a category was not provided by the user. Wouldn't it just be enough to log on the top-most level (Realm)? If so, the category ?? LogCategory.Realm below will handle that.

Suggested change
static setLogLevel(_arg: LogLevel | LogArgs) {
// It is not possible to overload a static function: https://github.com/microsoft/TypeScript/issues/18945
// FIXME: don't use `arguments` but find a proper type
if (arguments.length === 1) {
// eslint-disable-next-line prefer-rest-params
const arg = arguments[0];
if (arg.level) {
const level = arg.level;
if (arg.category) {
const category = arg.category;
assert(Object.values(LogCategory).includes(category));
binding.LogCategoryRef.getCategory(category).setDefaultLevelThreshold(toBindingLoggerLevel(level));
} else {
Object.values(LogCategory).forEach((category) => {
Realm.setLogLevel({ level, category });
});
}
} else {
const level = arg;
Object.values(LogCategory).forEach((category) => {
Realm.setLogLevel({ level, category });
});
}
} else {
throw new Error(`Wrong number of arguments - expected 1, got ${arguments.length}`);
}
}
static setLogLevel(level: LogLevel, category?: LogCategory) {
binding.LogCategoryRef.getCategory(category ?? LogCategory.Realm).setDefaultLevelThreshold(
toBindingLoggerLevel(level),
);
}

Even though the TS type defines the allowed categories, we may want to keep the assert you added where we check that the provided category is one of the allowed ones? But should add a friendly error message in that case 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have consider category as the second and optional argument. To me, it read that you set the category on the log level, which implies that you can only use a log level once. For example, I find it a bit harder to read:

Realm.setLogLevel(LogLevel.Debug, LogCategory.Realm);
Realm.setLogLevel(LogLevel.Debug, LogCategory.App);

and the following code has a lower cognitive load:

Realm.setLogLevel({ category: LogCategory.Realm, level: LogLevel.Debug });
Realm.setLogLevel({ category: LogCategory.App, level: LogLevel.Debug });

Copy link
Contributor

@elle-j elle-j Feb 2, 2024

Choose a reason for hiding this comment

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

I like that reasoning 🙂 One simplification could then be:

  static setLogLevel(options: LogLevel | { category: LogCategory; level: LogLevel }) {
    const { level, category } = typeof options === "string" ? { level: options, category: LogCategory.Realm } : options;
    binding.LogCategoryRef.getCategory(category).setDefaultLevelThreshold(toBindingLoggerLevel(level));
  }

* @example
* Realm.setLogger(({ category, level, message }) => {
* console.log(`[${category} - ${level}] ${message}`);
* });
*/
static setLogger(loggerCallback: LoggerCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative here is also to modify the LoggerCallback to e.g.:

export type Logger = (level: NumericLogLevel, message: string, category?: LogCategory) => void;

Then this function could be:

  static setLogger(loggerCallback: LoggerCallback) {
    const logger = binding.Helpers.makeLogger((category, level, message) => {
      loggerCallback(fromBindingLoggerLevelToLogLevel(level), message, category as LogCategory);
    });
    binding.Logger.setDefaultLogger(logger);
  }

@kneth kneth force-pushed the lj/core-next-major branch 2 times, most recently from 5e1a2eb to 8efb2ad Compare February 2, 2024 14:16
@kneth kneth marked this pull request as ready for review February 9, 2024 09:08
Copy link
Contributor

@elle-j elle-j left a comment

Choose a reason for hiding this comment

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

Nice updates! 🙂 Left some ideas and suggestions.

CHANGELOG.md Outdated
Comment on lines 20 to 41
<!-- * Upgraded Realm Core from vX.Y.Z to vA.B.C -->
* Fix Cocoapods to version 1.14.3 on Github Actions.
* Migrated bingen from `util::Optional` to `std::optional`.
* Fix Cocoapods to version 1.14.3 on Github Actions.
Copy link
Contributor

Choose a reason for hiding this comment

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

(Duplicate entry)

Suggested change
<!-- * Upgraded Realm Core from vX.Y.Z to vA.B.C -->
* Fix Cocoapods to version 1.14.3 on Github Actions.
* Migrated bingen from `util::Optional` to `std::optional`.
* Fix Cocoapods to version 1.14.3 on Github Actions.
* Migrated bindgen from `util::Optional` to `std::optional`.
* Fix Cocoapods to version 1.14.3 on Github Actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Binary uploaded

@@ -17,7 +17,7 @@
////////////////////////////////////////////////////////////////////////////

import { expect } from "chai";
import Realm, { List } from "realm";
import Realm, { LogCategory, LogArgs, LoggerCallbackArgs } from "realm";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import Realm, { LogCategory, LogArgs, LoggerCallbackArgs } from "realm";
import Realm, { LogCategory, LoggerCallbackArgs } from "realm";

}

/**
* Type for `Realm.setLogLevel`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Type for `Realm.setLogLevel`
* Log options to use for `Realm.setLogLevel`.

/**
* Type for `Realm.setLogLevel`
*/
export type LogArgs = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of using a name like LogOptions instead of args?

Comment on lines +171 to +184
export type LoggerCallbackArgs = {
category: LogCategory;
level: LogLevel;
message: string;
};
/**
* A callback passed to `Realm.setLogger`. Arguments are passed as a POJO.
*
* @param category - The category (origin) of the log entry.
* @param level - The level of the log entry.
* @param message - The message of the log entry.
* @since
*/
export type LoggerCallback2 = (args: LoggerCallbackArgs) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

One alternative here is to inline the types in LoggerCallbackArgs directly in the signature of LoggerCallback2 instead of having it as a separate type. Then we also don't need to export the argument types.

In the test files where you need to do const logger = (args: LoggerCallbackArgs) => { /*...*/ } you can update that to instead use the function type: const logger: Logger = (args) => { /*...*/ }.

(If you'd rather use one of the specific callbacks as the function type rather than the union Logger, you can do that as well.)

Comment on lines +222 to +223
export const defaultLogger: LoggerCallback = function (args: LoggerCallbackArgs) {
const { category, level, message } = args;
Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. Would be great to have a clearer distinction between the callback names 🙂

Suggested change
export const defaultLogger: LoggerCallback = function (args: LoggerCallbackArgs) {
const { category, level, message } = args;
export const defaultLogger: LoggerCallback2 = function (options) {
const { category, level, message } = options;

const bindingLoggerLevel = toBindingLoggerLevel(level);
binding.Logger.setDefaultLevelThreshold(bindingLoggerLevel);
// eslint-disable-next-line @typescript-eslint/no-unused-vars
static setLogLevel(_: LogLevel | LogArgs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think may be able to simplify the body a bit, while keeping the union parameter type. I'll refer to this comment and thread.

Copy link
Member

Choose a reason for hiding this comment

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

I agree ☝️ is a more clean solution.

Comment on lines +348 to 190
// This is a hack to check which of the two logger callbacks which are used
// It only works as the two callback type have different number of arguments, and it will
// probably produce odd error messages if the logger is set by `setLogger((...args) => console.log(args))`.
if (loggerCallback.length === 2) {
const cb = loggerCallback as LoggerCallback1;
logger = binding.Helpers.makeLogger((_category, level, message) => {
cb(fromBindingLoggerLevelToLogLevel(level), message);
});
} else {
const cb = loggerCallback as LoggerCallback2;
logger = binding.Helpers.makeLogger((category, level, message) => {
cb({ category: category as LogCategory, level: fromBindingLoggerLevelToLogLevel(level), message });
});
}
binding.Logger.setDefaultLogger(logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice check on the function's length 🙂

To prevent the misuse of (...args) as you mentioned, and to remove the need for as LoggerCallback1/2, we could throw for the wrong number of callback args and also extract the if-conditions into two functions in e.g. Logger.ts (with whatever names you think are suitable):

export function isLoggerWithLevel(logger: unknown): logger is LoggerCallback1 {
  return typeof logger === "function" && logger.length === 2;
}

export function isLoggerWithOptions(logger: unknown): logger is LoggerCallback2 {
  return typeof logger === "function" && logger.length === 1;
}

Then the setLogger() function could utilize those:

  static setLogger(loggerCallback: LoggerCallback) {
    let logger: binding.Logger;

    if (isLoggerWithLevel(loggerCallback)) {
      logger = binding.Helpers.makeLogger((_, level, message) => {
        loggerCallback(fromBindingLoggerLevelToLogLevel(level), message);
      });
    } else if (isLoggerWithOptions(loggerCallback)) {
      logger = binding.Helpers.makeLogger((category, level, message) => {
        loggerCallback({
          category: category as LogCategory,
          level: fromBindingLoggerLevelToLogLevel(level),
          message,
        });
      });
    } else {
      throw new Error(`Unexpected logger passed.`);
    }

    binding.Logger.setDefaultLogger(logger);
  }

Copy link
Member

Choose a reason for hiding this comment

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

I like the suggestion to use of an assert function, but it would be even greater if it used a pure function (loggerFromCallback or something) returning the logger instead of a local mutable variable 😉

@@ -60,7 +60,7 @@ export class Sync {

/** @deprecated Will be removed in v13.0.0. Please use {@link Realm.setLogger}. */
static setLogger(app: App, logger: Logger) {
const factory = binding.Helpers.makeLoggerFactory((level, message) => {
const factory = binding.Helpers.makeLoggerFactory((category, level, message) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logger received to Sync.setLogger() is currently (in our TS types) the one taking the numeric log level (thus, same as before). Core hasn't changed that API right?

Suggested change
const factory = binding.Helpers.makeLoggerFactory((category, level, message) => {
const factory = binding.Helpers.makeLoggerFactory((_, level, message) => {

Comment on lines +77 to +144
export enum LogCategory {
/**
* Include logs from all categories. Subcategories are {@link LogCategory.Storage},
* {@link LogCategory.Sync}, {@link LogCategory.App}, and {@link LogCategory.SDK}.
*/
Realm = "Realm",
/**
* Log database mutations and query operations.
* Subcategories are {@link LogCategory.Transaction}, {@link LogCategory.Object},
* {@link LogCategory.Query}, and {@link LogCategory.Notification}.
*/
Storage = "Realm.Storage",
/**
* Log when creating, advancing, and committing transactions.
*/
Transaction = "Realm.Storage.Transaction",
/**
* Log query operations.
*/
Query = "Realm.Storage.Query",
/**
* Log database mutations.
*/
Object = "Realm.Storage.Object",
/**
* Log notifications of changes to the database.
*/
Notification = "Realm.Storage.Notification",
/**
* Log activity related to Atlas Device Sync.
* Subcategories are {@link LogCategory.Client} and {@link LogCategory.Server}.
*/
Sync = "Realm.Sync",
/**
* Log activity related to Atlas Device Sync client operations.
* Subcategories are {@link LogCategory.Session}, {@link LogCategory.Changeset},
* {@link LogCategory.Network}, and {@link LogCategory.Reset}.
*/
Client = "Realm.Sync.Client",
/**
* Log connection level activity.
*/
Session = "Realm.Sync.Client.Session",
/**
* Log when receiving, uploading, and integrating changesets.
*/
Changeset = "Realm.Sync.Client.Changeset",
/**
* Log low level network activity.
*/
Network = "Realm.Sync.Client.Network",
/**
* Log client reset operations.
*/
Reset = "Realm.Sync.Client.Reset",
/**
* Log activity related to Atlas Device Sync server operations.
*/
Server = "Realm.Sync.Server",
/**
* Log activity at the Atlas App level.
*/
App = "Realm.App",
/**
* Log activity at the SDK level.
*/
SDK = "Realm.SDK",
}
Copy link
Member

@kraenhansen kraenhansen Feb 13, 2024

Choose a reason for hiding this comment

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

I feel the flattening of the namespace here is a bit misleading and would potentially result in future problems if two categories start using the same sub-category names. For example, what would we do if core introduce a Realm.App.Network category?

I'd rather that we keep this close to the Core API, use a string-with-dot notation and type this as a union of strings instead:

Suggested change
export enum LogCategory {
/**
* Include logs from all categories. Subcategories are {@link LogCategory.Storage},
* {@link LogCategory.Sync}, {@link LogCategory.App}, and {@link LogCategory.SDK}.
*/
Realm = "Realm",
/**
* Log database mutations and query operations.
* Subcategories are {@link LogCategory.Transaction}, {@link LogCategory.Object},
* {@link LogCategory.Query}, and {@link LogCategory.Notification}.
*/
Storage = "Realm.Storage",
/**
* Log when creating, advancing, and committing transactions.
*/
Transaction = "Realm.Storage.Transaction",
/**
* Log query operations.
*/
Query = "Realm.Storage.Query",
/**
* Log database mutations.
*/
Object = "Realm.Storage.Object",
/**
* Log notifications of changes to the database.
*/
Notification = "Realm.Storage.Notification",
/**
* Log activity related to Atlas Device Sync.
* Subcategories are {@link LogCategory.Client} and {@link LogCategory.Server}.
*/
Sync = "Realm.Sync",
/**
* Log activity related to Atlas Device Sync client operations.
* Subcategories are {@link LogCategory.Session}, {@link LogCategory.Changeset},
* {@link LogCategory.Network}, and {@link LogCategory.Reset}.
*/
Client = "Realm.Sync.Client",
/**
* Log connection level activity.
*/
Session = "Realm.Sync.Client.Session",
/**
* Log when receiving, uploading, and integrating changesets.
*/
Changeset = "Realm.Sync.Client.Changeset",
/**
* Log low level network activity.
*/
Network = "Realm.Sync.Client.Network",
/**
* Log client reset operations.
*/
Reset = "Realm.Sync.Client.Reset",
/**
* Log activity related to Atlas Device Sync server operations.
*/
Server = "Realm.Sync.Server",
/**
* Log activity at the Atlas App level.
*/
App = "Realm.App",
/**
* Log activity at the SDK level.
*/
SDK = "Realm.SDK",
}
export type LogCategory = "Realm" | "Realm.Storage" | "Realm.Storage.Transaction" /* ... */;

The only drawback is that it doesn't seem to be possible to document this using a tsdoc string. But we could probably document the different values on the LogCategory type itself or the individual methods taking the value 🤔

}
} else {
const level = arg as LogLevel;
Object.values(LogCategory).forEach((category) => {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be possible to just set the log level for the "Realm" and let Core handle updating the sub-categories? (at lease that was my take-away from our conversation yesterday).

};

// FIXME: don't use `arguments` but find a proper type
if (arguments.length === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (arguments.length === 1) {
if (typeof _ !== "string") {

const bindingLoggerLevel = toBindingLoggerLevel(level);
binding.Logger.setDefaultLevelThreshold(bindingLoggerLevel);
// eslint-disable-next-line @typescript-eslint/no-unused-vars
static setLogLevel(_: LogLevel | LogArgs) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest a better name and referring to it directly instead of via arguments[0].

Suggested change
static setLogLevel(_: LogLevel | LogArgs) {
static setLogLevel(argument: LogLevel | LogArgs) {


Realm.setLogLevel("all");
Realm.setLogger(logger);
Realm.setLogLevel({ category: LogCategory.Realm, level: "all" });
Copy link
Member

@kraenhansen kraenhansen Feb 13, 2024

Choose a reason for hiding this comment

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

If I understand the proposed changes correctly, simply passing a level to setLogLevel isn't deprecated, in which case I'd suggest leaving the tests as is and adding new tests for the new behaviour (potentially duplicating code or by extracting some common code into a function).

* @since
*/
export type LoggerCallback2 = (args: LoggerCallbackArgs) => void;
export type LoggerCallback = LoggerCallback1 | LoggerCallback2;
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I'm actually not convinced the change in Core API justifies adding an overload to the callback passed to setLogger 🤔 Especially because we need to rely on arguments.length to figure out which callback the user has passed ...

Is passing the category as a third positional argument really that awful?

Realm.setLogger((level, message, category) => {
  // ...
});

@kneth kneth closed this Apr 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants