[bindgen] Refactor namespaces #5376

kraenhansen · 2023-02-06T12:47:42Z

What, How & Why?

This refactors existing namespace code and adds missing members.

I basically went though out root ./types directory to verify that all we exported before is now also being exported both as named exports and through the Realm namespace.

I noted suggestions for cases where we intentionally break the API in the project's technical design doc (internal to MongoDB employees).

kraenhansen · 2023-02-06T12:50:07Z

packages/realm/src/index.ts

-export type {
-  CollectionChangeCallback,
-  CollectionChangeSet,
-  Configuration,
-  FlexibleSyncConfiguration,
-  ObjectChangeCallback,
-  ObjectChangeSet,
-  ObjectSchema,
-  PropertySchema,
-  PartitionSyncConfiguration,
-  RealmEventName,
-  RealmListenerCallback,
-  SubscriptionOptions,
-} from "./internal";


@elle-j I remember you mentioned that you had errors when exporting these without the type keyword. Do you remember when that error occurred? This builds for me without issues, so I wonder if it was somewhere when running the tests?

Yes that builds for me as well, but the error occurred when running our unit tests in realm.

cd packages/realm npm test

I've made a few changes and verified that switching to tsx as the TS runtime makes the SDK unit tests run again: #5378

kraenhansen · 2023-02-06T14:21:01Z

packages/realm/src/app-services/App.ts

@@ -119,6 +149,8 @@ export class App {
        platformVersion: App.PLATFORM_VERSION,
        sdkVersion: App.SDK_VERSION, // Used to be "RealmJS/" + SDK_VERSION
        transport: createNetworkTransport(),
+        localAppName: app?.name,
+        localAppVersion: app?.version,


I noticed the implementation for this was missing and since it was literally a 1 minute job, I just did it in this PR 🙈

This is to avoid breaking the API just yet.

elle-j

Nice effort making these changes! Deprecating the namespace types definitely seems like the way to go. (Also, thanks for fixing some typos here and there in comments.)

Left some questions and suggestions, after that it'll be awesome getting this merged.

elle-j · 2023-02-06T14:12:16Z

packages/realm/src/Dictionary.ts

@@ -282,3 +275,8 @@ export class Dictionary<T = unknown> extends Collection<string, T, [string, T],
    );
  }
 }
+
+export interface Dictionary<T = unknown> {


Since class Dictionary is exported, why do we need to export an interface as well?

Actually. This change is not strictly needed anymore. The types are merging, but I'll revert this, since thats not 100% obvious.

elle-j · 2023-02-06T14:22:45Z

packages/realm/src/Realm.ts

+  // TODO: Decide if we want to deprecate this as well
+  public static Object = RealmObject;


I'd think that having all types be imported in the same way (named) would be less confusing (and more consistent), especially if Results, Dictionary, etc. are deprecated. What would be the reason for not deprecating it?

Only that Object is a class provided by the runtime globally and import { Object } from "realm" would shadow that (and confuse some static analysis tools, like linters, etc.). In a file where an end-user need access to the Object provided by the runtime and that provided by us, they could rename on import:

import { Object as RealmObject } from "realm"; // Use RealmObject

Or alternatively import us as a namespace (which won't be deprecated):

import * as Realm from `realm`; // Use Realm.Object

The latter could be a bit confusing since Realm in that case wouldn't be a constructor.

By not deprecating on the line we're discussing, we would allow:

import { Realm } from "realm"; // Use Realm.Object // Realm is still a constructor

I suspect this is the reason the namespaced API got introduced in the first case.

Gotcha, thanks for the explanation. One more thing, in index.ts we export e.g. RealmObject as Object, could we not export it as RealmObject instead (but not in v12)?

could we not export it as RealmObject

We could .. We should probably think long an hard about that tough. The package name ("realm") is already a namespace (not the TS kind of namespace ...), so in general I tend to opt towards not include a name twice (both in the thing and the thing / namespace that contains it)

elle-j · 2023-02-06T14:29:49Z

packages/realm/src/Realm.ts

  public static User = User;
-  public static Credentials = Credentials;


Will this be a breaking change? (Removing Credentials from Realm and its namespace, and adding it to App)

Great catch. Because of https://github.com/realm/realm-js/tree/bindgen/types#L266-L269 I thought they were exported on App only, but looking more closely, this should definitely not be removed.

elle-j · 2023-02-06T14:36:45Z

packages/realm/src/Realm.ts

@@ -107,20 +165,54 @@ type InternalConfig = {
 };

 export class Realm {


So much easier to read and navigate when the members are in alphabetical order, thanks 😃

elle-j · 2023-02-06T14:55:20Z

packages/realm/src/Realm.ts

+    /** @deprecated Please use named imports */
+    CanonicalObjectSchemaProperty,
+    /** @deprecated Please use named imports */
+    CanonicalPropertiesTypes,
+    /** @deprecated Please use named imports */


I saw that the old API exports CanonicalObjectSchemaProperty and CanonicalPropertiesTypes (and CanonicalObjectSchema, this last one is missing from this namespace), but I'm not sure why we're exporting the canonical types as opposed to only the "regular/relaxed" ones.

Would it not be preferable to remove those 3 canonical public exports in v13 (meaning, not only from the namespace but also not providing them as named exports)? (And in this case mention this in the deprecation comments.)

The only differences between the publicly exported..

(A) CanonicalObjectSchema vs. (B) ObjectSchema:

(A) properties: The values use object notation.

(B) properties: The values use either object or shorthand notation.

(A) CanonicalObjectSchemaProperty vs. (B) ObjectSchemaProperty:

(A) name: The name is provided.

(B) name: The name is not provided (it's just the same as the property name).

(A) CanonicalPropertiesTypes vs. (B) PropertiesTypes:

(A) The values use object notation.

(B) The values use either object or shorthand notation.

Then, for methods that return canonical types (e.g. Realm.schema) we could change those to return the relaxed ones.

for methods that return canonical types (e.g. Realm.schema) we could change those to return the relaxed ones.

From my work on Studio I found it nicer to program against the canonical types. Granted, most end-user applications doesn't do schema reflection like Studio does, but for a CMS like front-end to a database, having the canonical types as well as having the Realm#schema be canonical is actually a massive upside, since the consuming code needs to consider less edge-cases.

Interesting, point well taken! :) (ps. I believe CanonicalObjectSchema needs to be added there as well then)

elle-j · 2023-02-06T15:51:45Z

packages/realm/src/binding.ts

@@ -52,12 +52,14 @@ Timestamp.prototype.toDate = function () {
  return new Date(Number(this.seconds) * 1000 + this.nanoseconds / 1000_000);
 };

+/** @internal */


elle-j · 2023-02-06T15:55:37Z

packages/realm/src/errors.ts

+/**
+ * @deprecated Use the another {@link ClientResetMode} than {@link ClientResetMode.Manual}.
+ */
 export class ClientResetError extends SyncError {


What's the correlation between ClientResetMode.Manual and ClientResetError in this case? Perhaps it could be clarified a bit here.

To be honest, I'm lacking context myself, but I've added links to the changelog and original PR adding the deprecation in the first place. We could probably revisit this.

@kneth do you remember why this got deprecated? Is it mainly to discourage the use of the manual client reset mode?

elle-j · 2023-02-06T16:05:51Z

packages/realm/src/index.ts

@@ -18,44 +18,110 @@

 export {


These types could be helpful for users as well:

PropertySchemaStrict

PropertyTypeName

PrimitivePropertyTypeName

CollectionPropertyTypeName

RelationshipPropertyTypeName

UserTypeName

elle-j · 2023-02-06T16:09:49Z

packages/realm/src/index.ts

-export type {
-  CollectionChangeCallback,
-  CollectionChangeSet,
-  Configuration,
-  FlexibleSyncConfiguration,
-  ObjectChangeCallback,
-  ObjectChangeSet,
-  ObjectSchema,
-  PropertySchema,
-  PartitionSyncConfiguration,
-  RealmEventName,
-  RealmListenerCallback,
-  SubscriptionOptions,
-} from "./internal";


Yes that builds for me as well, but the error occurred when running our unit tests in realm.

cd packages/realm npm test

elle-j · 2023-02-06T17:10:45Z

packages/realm/src/tests/milestone-4.test.ts

@@ -18,7 +18,8 @@

 import { expect } from "chai";

-import { Realm } from "../index";
+import { Realm, Results } from "../index";


I've noticed that the tests usually import from individual files rather than from internal.ts. Should we start using internal.ts for the tests as well or does it not matter much in this case?

That would probably be good, but then again, I expect to delete these or partly migrate them to integration tests soon.

elle-j

Great 💯

elle-j · 2023-02-07T09:31:27Z

packages/realm/src/Realm.ts

+    /** @deprecated Please use named imports */
+    CanonicalObjectSchemaProperty,


We can refer the user here to the now-called CanonicalPropertySchema (also needs to be imported at the top).

Suggested change

/** @deprecated Please use named imports */

CanonicalObjectSchemaProperty,

/** @deprecated Will be removed in v13.0.0. Please use {@link CanonicalPropertySchema} as a named import */

CanonicalObjectSchemaProperty,

/** @deprecated Please use named imports */

CanonicalPropertySchema,

Feel free to rearrange/reformulate.

And added a link to it on the CanonicalObjectSchemaProperty.

* bindgen: [bindgen] Migrated `tsm` → `tsx` (#5378) [bindgen] Refactor namespaces (#5376) Fixed space Small correction to docs # Conflicts: # packages/realm/src/Dictionary.ts

@ts-expect-error

* WIP on improving the type-tests * Removed a stray comment start * Fixed missing @ts-expect-error in bundle.d.ts * Propagate local app name and version * Renamed AuthClients to Auth This is to avoid breaking the API just yet. * Refactored namespaces * Fixed namespaced Types export * Fixed InsertionModel types * Fixed a type-o * Ensuring named exports are correct * Updated unit tests to import from ../index * Fixed some exported types * Merged Dictionary interface and class * Reverting removal of `Realm.Credentials` * Reordered and exporting MutableSubscriptionSet * Updated AppConfiguration#baseUrl docs * Adding links to ClientResetError deprecation * Exporting types pr review * Adding "CanonicalObjectSchema" to Realm namespace * Export CanonicalPropertySchema And added a link to it on the CanonicalObjectSchemaProperty.

@ts-expect-error

* WIP on improving the type-tests * Removed a stray comment start * Fixed missing @ts-expect-error in bundle.d.ts * Propagate local app name and version * Renamed AuthClients to Auth This is to avoid breaking the API just yet. * Refactored namespaces * Fixed namespaced Types export * Fixed InsertionModel types * Fixed a type-o * Ensuring named exports are correct * Updated unit tests to import from ../index * Fixed some exported types * Merged Dictionary interface and class * Reverting removal of `Realm.Credentials` * Reordered and exporting MutableSubscriptionSet * Updated AppConfiguration#baseUrl docs * Adding links to ClientResetError deprecation * Exporting types pr review * Adding "CanonicalObjectSchema" to Realm namespace * Export CanonicalPropertySchema And added a link to it on the CanonicalObjectSchemaProperty.

kraenhansen self-assigned this Feb 6, 2023

kraenhansen requested a review from elle-j February 6, 2023 12:47

cla-bot bot added the cla: yes label Feb 6, 2023

kraenhansen commented Feb 6, 2023

View reviewed changes

kraenhansen added 10 commits February 6, 2023 15:37

WIP on improving the type-tests

0ceb8d8

Removed a stray comment start

4d0ee40

Fixed missing @ts-expect-error in bundle.d.ts

94b7be4

Propagate local app name and version

93667ce

Renamed AuthClients to Auth

9b9d487

This is to avoid breaking the API just yet.

Refactored namespaces

93e3fef

Fixed namespaced Types export

a17d575

Fixed InsertionModel types

49cd397

Fixed a type-o

427a901

Ensuring named exports are correct

92ba2a9

kraenhansen force-pushed the kh/bindgen/refactor-namespaces branch from 3750aa5 to 92ba2a9 Compare February 6, 2023 14:37

elle-j reviewed Feb 6, 2023

View reviewed changes

kraenhansen added 8 commits February 6, 2023 21:48

Updated unit tests to import from ../index

3c9c917

Fixed some exported types

bd591bb

Merged Dictionary interface and class

d7d0841

Reverting removal of Realm.Credentials

7699c4b

Reordered and exporting MutableSubscriptionSet

c10beec

Updated AppConfiguration#baseUrl docs

34dd9e4

Adding links to ClientResetError deprecation

27e39cc

Exporting types pr review

a980778

kraenhansen requested a review from elle-j February 6, 2023 21:18

elle-j approved these changes Feb 7, 2023

View reviewed changes

kraenhansen added 2 commits February 7, 2023 13:08

Adding "CanonicalObjectSchema" to Realm namespace

5315a9e

Export CanonicalPropertySchema

aa2b3ba

And added a link to it on the CanonicalObjectSchemaProperty.

kraenhansen merged commit 712a277 into bindgen Feb 7, 2023

kraenhansen deleted the kh/bindgen/refactor-namespaces branch February 7, 2023 12:15

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bindgen] Refactor namespaces #5376

[bindgen] Refactor namespaces #5376

kraenhansen commented Feb 6, 2023

kraenhansen Feb 6, 2023

elle-j Feb 6, 2023 •

edited

Loading

kraenhansen Feb 6, 2023

kraenhansen Feb 6, 2023

elle-j Feb 6, 2023

elle-j left a comment

elle-j Feb 6, 2023

kraenhansen Feb 6, 2023

elle-j Feb 6, 2023

kraenhansen Feb 6, 2023

elle-j Feb 7, 2023 •

edited

Loading

kraenhansen Feb 7, 2023

elle-j Feb 6, 2023

kraenhansen Feb 6, 2023 •

edited

Loading

elle-j Feb 6, 2023

elle-j Feb 6, 2023 •

edited

Loading

kraenhansen Feb 6, 2023

elle-j Feb 7, 2023

elle-j Feb 6, 2023

elle-j Feb 6, 2023

kraenhansen Feb 6, 2023

elle-j Feb 6, 2023

elle-j Feb 6, 2023 •

edited

Loading

elle-j Feb 6, 2023

kraenhansen Feb 6, 2023

elle-j left a comment

elle-j Feb 7, 2023

		// TODO: Decide if we want to deprecate this as well
		public static Object = RealmObject;

		public static User = User;
		public static Credentials = Credentials;

		@@ -107,20 +165,54 @@ type InternalConfig = {
		};

		export class Realm {

		/** @deprecated Please use named imports */
		CanonicalObjectSchemaProperty,

[bindgen] Refactor namespaces #5376

[bindgen] Refactor namespaces #5376

Conversation

kraenhansen commented Feb 6, 2023

What, How & Why?

Choose a reason for hiding this comment

elle-j Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Feb 6, 2023 •

edited

Loading

elle-j Feb 7, 2023 •

edited

Loading

kraenhansen Feb 6, 2023 •

edited

Loading

elle-j Feb 6, 2023 •

edited

Loading

elle-j Feb 6, 2023 •

edited

Loading