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

Prepare for 12.0.0-alpha.0 #5600

Merged
merged 5 commits into from
Mar 22, 2023
Merged

Prepare for 12.0.0-alpha.0 #5600

merged 5 commits into from
Mar 22, 2023

Conversation

realm-ci
Copy link
Contributor

An automated PR for next release.

Copied notes from the technical design on breakages to the API.
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM. Just some grammar things.

* In an effort to align with EcmaScript modules, we’re adopting a pattern of named exports on the “root” of the package namespace. This change also affects how our library is imported on CommonJS / Node.js: Before, we exported our constructor directly from the package (`module.export = Realm` style), which would allow CommonJS runtimes like Node.js to import us using a simple assignment:
```javascript
const Realm = require(“realm”); // this commonjs style - won’t work anymore
From now on, users need to consume us using a named import, like this:
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
From now on, users need to consume us using a named import, like this:
// From now on, users need to consume us using a named import, like this:

Copy link
Contributor

Choose a reason for hiding this comment

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

or end the code block and start another after the comment

CHANGELOG.md Outdated
```
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
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
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we haven't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.

CHANGELOG.md Outdated
import Realm from “realm”; // esm style - consuming our “default” export
```
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
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
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See [#4934](https://github.com/realm/realm-js/issues/4934).

This didn't resolve to a link in the file viewer

CHANGELOG.md Outdated
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
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
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which internally extends `Collection`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even though internally is also correct, I believe the intended meaning here is in turn.

Copy link
Member

Choose a reason for hiding this comment

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

Right - I ment to write "in turn", but I opted for Kenneths suggestion of removing the word all together.

CHANGELOG.md Outdated
{ type: "object[]", objectType: "SomeObject" } // No longer works
{ type: "list", objectType: "SomeObject" } // This still works
```
* To prevent modifying end-users class-based model classes, we’re now wrapping class-based models in our own model. This is technically not a breaking change, as the previous behavior was undocumented and while objects will still pass `instanceof SomeClass` checks, code who is testing using prototype comparisons will fail:
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
* To prevent modifying end-users class-based model classes, we’re now wrapping class-based models in our own model. This is technically not a breaking change, as the previous behavior was undocumented and while objects will still pass `instanceof SomeClass` checks, code who is testing using prototype comparisons will fail:
* To prevent modifying end-users class-based model classes, we’re now wrapping class-based models in our own model. This is technically not a breaking change, as the previous behavior was undocumented and while objects will still pass `instanceof SomeClass` checks, code that is testing using prototype comparisons will fail:

CHANGELOG.md Outdated
```js
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
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
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary. This is undocumented behaviour that we were testing for which makes little sense in practice and hence won’t be implemented in the new SDK.

Copy link
Contributor

@bmunkholm bmunkholm Mar 21, 2023

Choose a reason for hiding this comment

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

we suggest deprecating

we suggest or we are? I assume we have already done something and have a default choice?

Copy link
Member

Choose a reason for hiding this comment

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

I opted for Mathias' suggestion.

CHANGELOG.md Outdated
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
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
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`. Therefore, we are deprecating the API with the @deprecation flag and a single console.warn of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!

Copy link
Member

Choose a reason for hiding this comment

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

I've created #5605 to track the actual deprecation as I'm actually unsure if we ever did that.

CHANGELOG.md Outdated
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
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
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view. We've deprecated this on v11 and removed it from v12.

CHANGELOG.md Outdated
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
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
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
* We’ve decided to remove numeric indexing and “array methods” from the `SubscriptionSet`, since it would bloat our SDK code, the team saw little actual use-case for it, we expect very few developers were actually using this and the workaround is really easy to implement (spreading into an array `[...subscriptions]`). Again something we would love feedback on.

CHANGELOG.md Outdated

### Enhancements
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still have a spread operator issue open? If so, this would probably be a good place to link it.

Copy link
Member

Choose a reason for hiding this comment

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

Great idea - I've added those.

CHANGELOG.md Outdated
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "interns"

CHANGELOG.md Outdated
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
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
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team sees little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.

CHANGELOG.md Outdated

### Breaking changes

Although this is a complete re-write of our SDK, we've strived to keep breakages to a minimum and expect our users to upgrade from v11 without any notible changes to their code-base.
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
Although this is a complete re-write of our SDK, we've strived to keep breakages to a minimum and expect our users to upgrade from v11 without any notible changes to their code-base.
Although this is a complete rewrite of our SDK, we've strived to keep breakages to a minimum and expect our users to upgrade from v11 without any notable changes to their code-base.

However, is this really true? Aren't we listing some notable changes that users will need to make? Perhaps we should say "significant" rather than "notable"?

Copy link
Contributor

@elle-j elle-j Mar 21, 2023

Choose a reason for hiding this comment

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

+1 ☝️ to the above comment.

Copy link
Contributor

@RedBeard0531 RedBeard0531 left a comment

Choose a reason for hiding this comment

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

Sending out first batch of comments before heading to a meeting.

CHANGELOG.md Outdated
import { Realm } from “realm”; // esm style
import Realm from “realm”; // esm style - consuming our “default” export
```
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
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
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK. Also, since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆

A) what link?
B) "since its inception" is a bit ambigous. Do you mean "since we initially designed the realm-js API" or "since TS added namespaces".

Copy link
Member

Choose a reason for hiding this comment

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

A) in the note in the top of the change log entry. I could clarify that with a link here as well.
B) It predates TS namespaces too. I'll clarify.

CHANGELOG.md Outdated
Comment on lines 15 to 16
import { Realm } from “realm”; // esm style
import Realm from “realm”; // esm style - consuming our “default” export
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 } from “realm”; // esm style
import Realm from “realm”; // esm style - consuming our “default” export
import { Realm } from “realm”; // esm style
import * as realm from "realm"; // esm namespaced style for more explicit usage like realm.List and realm.Object
import Realm from “realm”; // esm style - consuming our “default” export

I also added a blank line to separate cjs from esm examples, since they probably shouldn't be mixed.

CHANGELOG.md Outdated
import Realm from “realm”; // esm style - consuming our “default” export
```
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we didn't want to use the gfm-specific #123-style shortcut links since they don't work everywhere that we put these anouncements.

CHANGELOG.md Outdated
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
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
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which in turn extends `Collection`.

Maybe mention that OrderedCollection and Collection should be considered internal types and not used by user code?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think they are 🤔 There might be some situations where users want some function to take anything ordered and would want to use those types. If we considered them internal we shouldn't export them.

Copy link
Contributor

Choose a reason for hiding this comment

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

What use case is there for users depending on the realm-specific OrderedCollection type? If they want "anything ordered", they are probably also fine with a plain JS array. I would expect most users to be better served with either ReadOnlyArray<T> or even Iterable<T> if they only need iteration.

CHANGELOG.md Outdated
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
* Users will no longer be able to mix declaring schemas using a shorthand (string) (ex “object[]” for a list of objects) and declaring an “objectType” in their schema. They have to choose one and stick with it for every property. This is a minor breaking change since this used to be allowed:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also now disallow types like {type: "int[]"}, so objectType has nothing to do with it. It is that you can only use the shorthand with strings, not as the type field in an object.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to clarify this:

  • Users will no longer be able to declare schemas using a shorthand (string) (ex "object[]" for a list of objects) when using an object to declare their schema. Per property, you have to choose either a shorthand string or an object with a non-shorthand type. This is a minor breaking change since this used to be allowed:
{ type: "object[]", objectType: "SomeObject" } // No longer works
{ type: "list", objectType: "SomeObject" } // This still works

Copy link
Member

Choose a reason for hiding this comment

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

Or actually - I choose LJ's proposal instead.

CHANGELOG.md Outdated
{ type: "object[]", objectType: "SomeObject" } // No longer works
{ type: "list", objectType: "SomeObject" } // This still works
```
* To prevent modifying end-users class-based model classes, we’re now wrapping class-based models in our own model. This is technically not a breaking change, as the previous behavior was undocumented and while objects will still pass `instanceof SomeClass` checks, code who is testing using prototype comparisons will fail:
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
* To prevent modifying end-users class-based model classes, we’re now wrapping class-based models in our own model. This is technically not a breaking change, as the previous behavior was undocumented and while objects will still pass `instanceof SomeClass` checks, code who is testing using prototype comparisons will fail:
* To prevent modifying end-users' class-based model classes, we’re now wrapping class-based models in our own model. Objects will still pass `instanceof SomeClass` checks, however, code which is directly using prototype or constructor comparisons will fail:

We shouldn't use the "it was undocumented so it isn't technically a breaking change" excuse, unless we explicitly documented that some behavior isn't guaranteed. Someone's app used to work and now doesn't. If we are making a change, we should own it.

```
* To prevent modifying end-users class-based model classes, we’re now wrapping class-based models in our own model. This is technically not a breaking change, as the previous behavior was undocumented and while objects will still pass `instanceof SomeClass` checks, code who is testing using prototype comparisons will fail:
```js
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
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
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
object.constructor == CustomObject // No longer works

CHANGELOG.md Outdated
```js
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
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
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* Symbols used to be accepted as keys in a dictionary, where they were coerced to strings prior to performing lookup. This was undocumented behaviour that makes little sense in practice (and arguably defeats the main purpose of the JS `Symbol` type). In the new SDK, using a Symbol as a key in a dictionary will throw.

Please double check and possibly fix the last sentence. I think we should describe what happens if someone already has an app doing this, rather than just saying not supported.

Copy link
Member

Choose a reason for hiding this comment

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

I've created #5604 to track that.

CHANGELOG.md Outdated
Object.getPrototypeOf(object) == CustomObject.prototype // No longer works
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
Copy link
Contributor

@RedBeard0531 RedBeard0531 Mar 21, 2023

Choose a reason for hiding this comment

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

Suggested change
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and introduced `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods as a workaround. The new SDK wraps its accessor objects in a Proxy, trapping the ownKeys operation which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` to work correctly, with minimal performance impact on normal accesses. Therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!

we suggest deprecating the API

That doesn't really make sense. "we suggest" implies that we recomend that the user takes some action, but I don't think we want users to modify our code. Do you mean something like "We are considering...", or more like "We suggest migrating away from those APIs even though we haven't yet decided if we will be officially deprecating them"

Copy link
Member

Choose a reason for hiding this comment

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

It was a copy-paste mistake. The technical design was written as a proposal.

CHANGELOG.md Outdated
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
Copy link
Contributor

@RedBeard0531 RedBeard0531 Mar 21, 2023

Choose a reason for hiding this comment

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

Suggested change
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* The [push service](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) has already been deprecated on the Atlas server. We've deprecated this on v11 and removed it from v12.

Copy link
Contributor

Choose a reason for hiding this comment

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

from the should also be removed in the suggested change 👍

CHANGELOG.md Outdated
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally, not expected to be used by users and most are very simple to type out for any user relying on it. Similarly the `DictionaryBase` is a type that was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident. We propose removing this and ask users to simply use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) on the `Realm.Sync.Session`. We’re no longer using the `*Payload` types (were actually only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.
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
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally, not expected to be used by users and most are very simple to type out for any user relying on it. Similarly the `DictionaryBase` is a type that was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident. We propose removing this and ask users to simply use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) on the `Realm.Sync.Session`. We’re no longer using the `*Payload` types (were actually only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally, not expected to be used by users and most are very simple to type out for any user relying on it. Similarly the `DictionaryBase` is a type that was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident. We propose removing this and ask users to simply use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) as `Realm.Sync.Session`. We’re no longer using the `*Payload` types (they were only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.

We propose removing

That doesn't sound right. Who are we proposing this to? Our users? Maybe use "We are considering" or "We are planning on" depending on what you are trying to say here.

Copy link
Member

Choose a reason for hiding this comment

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

It was a copy paste slip through, the technical design was written more as a proposal.

* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally, not expected to be used by users and most are very simple to type out for any user relying on it. Similarly the `DictionaryBase` is a type that was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident. We propose removing this and ask users to simply use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) on the `Realm.Sync.Session`. We’re no longer using the `*Payload` types (were actually only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.
* The return values of Object#getPropertyType was changed to return `"list"` instead of `"array"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to use RealmList or at least List (capital L) here? Ditto for RealmObject or Realm.Object, since Object seems ambiguous.

Copy link
Member

Choose a reason for hiding this comment

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

No. It's returns strings similar to those the schema parser takes.

CHANGELOG.md Outdated

### Enhancements
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`
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
* The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`
* The new SDK supports operations like `Object.keys(obj)` and the spread operator `{...obj}` on `RealmObject`s

The Proxy usage is an internal implementation detail. We should only say what is supported, not how we did so.

We may also want to include a note like "We still recommend explicitly only accessing the fields you need rather than using spreads. Spreads eagerly access all fields in the object, which can have significant performance costs if some fields are not actually needed."

CHANGELOG.md Outdated

### Enhancements
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
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
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`, which defaults to false. When set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.

I assumed the default based on the phrasing here, without actually checking. Please fix if I guessed wrongly.


### Enhancements
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* Added an overload to `Object.linkingObjects` method that takes type of the linking object as an input instead of its string name ([#5326](https://github.com/realm/realm-js/issues/5326))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we go through the whole document and pick a consistent way to refer to Realm.Object (or RealmObject, or realm.Object, ...)? I don't think we should use unqualified Object since that clashes with the js Object constructor object. We probably should consider renaming it in our own code as well for the same reason.

Copy link
Member

@kraenhansen kraenhansen Mar 21, 2023

Choose a reason for hiding this comment

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

I agree - we should converge on one way to refer to it.

It's called RealmObject in our own code, except before the final export for backwards compatibility (and because it's shorter).

@@ -33,6 +66,7 @@ realm.write(() => {
* File format: generates Realms with format v23 (reads and upgrades file format v5 or later for non-synced Realm, upgrades file format v10 or later for synced Realms).
Copy link
Contributor

Choose a reason for hiding this comment

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

It won't let me comment on the above lines, but there are some issues.

  • don't we depend on RN >= 0.71.something?
  • remove the dummy line in Fixed (or actually fill it in with some of the bugs we think we fixed...)

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.

Great effort and detail! Added some suggestions.

CHANGELOG.md Outdated

### Breaking changes

Although this is a complete re-write of our SDK, we've strived to keep breakages to a minimum and expect our users to upgrade from v11 without any notible changes to their code-base.
Copy link
Contributor

@elle-j elle-j Mar 21, 2023

Choose a reason for hiding this comment

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

+1 ☝️ to the above comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent bugs from copying code snippets, you can search and replace all with ", as well as with ' 👍 (or at least within the code blocks)

CHANGELOG.md Outdated
* Similarly we’re deprecating our namespaced API (the long chaining of identifiers, such as Realm.App.Sync.setLogLevel) in favor of the shorter named exports. Our main motivation is that it has proven very verbose and hard to maintain in our new SDK and since its inception (which predates ES Modules), we believe the community has moved towards a preference for simple named exports. We would love your feedback on this decision - please comment on the discussion linked above 👆
* The entire BSON package used to be re-exported as Realm.BSON, to simplify the new SDK we want to export only the BSON types that our SDK database component supports (ObjectId, Decimal128 and UUID). See #4934.
* We're now reusing code to perform assertions and although this is strictly not a breaking change, since we havn't historically documented error messages, you should probably revisit any code in your app which relies on matching on specific error messages.
* `Results`, `List` and `Set` used to inherit directly from `Collection` but now inherits from an abstract `OrderedCollection`, which interns extends `Collection`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though internally is also correct, I believe the intended meaning here is in turn.

CHANGELOG.md Outdated
Comment on lines 22 to 26
* Users will no longer be able to mix declaring schemas using a shorthand (string) (ex “object[]” for a list of objects) and declaring an “objectType” in their schema. They have to choose one and stick with it for every property. This is a minor breaking change since this used to be allowed:
```js
{ type: "object[]", objectType: "SomeObject" } // No longer works
{ type: "list", objectType: "SomeObject" } // This still works
```
Copy link
Contributor

@elle-j elle-j Mar 21, 2023

Choose a reason for hiding this comment

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

Adding some specificity:

Suggested change
* Users will no longer be able to mix declaring schemas using a shorthand (string) (ex “object[]” for a list of objects) and declaring an “objectType” in their schema. They have to choose one and stick with it for every property. This is a minor breaking change since this used to be allowed:
```js
{ type: "object[]", objectType: "SomeObject" } // No longer works
{ type: "list", objectType: "SomeObject" } // This still works
```
* In order to better guide users toward correct usage and understanding of the Realm property types, users must now be explicit about the property type when declaring object schemas. Additionally, mixing shorthand (string) and object representation for the property type is no longer permitted. (See the `PropertySchema` and `PropertySchemaShorthand` types.)
```js
// Example object schema
const TaskSchema = {
name: "Task",
properties: {
description: /* property schema (shorthand or object form) */,
},
};
// Explicitness
"[]" // Bad (previously parsed as implicit "mixed")
"mixed[]" // Good
{ type: "list" } // Bad
{ type: "list", objectType: "mixed" } // Good
// Mixing shorthand and object form
{ type: "int[]" } // Bad
"int[]" // Good
{ type: "list", objectType: "int" } // Good
{ type: "int?" } // Bad
"int?" // Good
{ type: "int", optional: true } // Good
// Specifying object types
{ type: "SomeType" } // Bad
"SomeType" // Good
{ type: "object", objectType: "SomeType" } // Good
{ type: "object[]", objectType: "SomeType" } // Bad
"SomeType[]" // Good
{ type: "list", objectType: "SomeType" } // Good
{ type: "linkingObjects", objectType: "SomeType", property: "someProperty" } // Good

CHANGELOG.md Outdated
```
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
Copy link
Contributor

Choose a reason for hiding this comment

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

from the should also be removed in the suggested change 👍

CHANGELOG.md Outdated
* It used to be the case that Symbols got type coerced to string when used as keys into a dictionary, this is undocumented behaviour that we were testing for which makes little sense in practice, hence won’t be implemented in the new SDK.
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
Copy link
Contributor

Choose a reason for hiding this comment

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

Building on top of Andrew's and Kenneth's suggestions.
(I removed the we expect very few developers.. since it seems similar to the team sees little actual use-case..)

Suggested change
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since (a) the team sees little actual use-case for it, (b) it would bloat our SDK code, and (c) there is a simple workaround if needed (spreading into an array `[...realm.subscriptions]`). (The property `length` is available.) Again, something we would love feedback on.

CHANGELOG.md Outdated
* As a part of migrating to NAPI (since ~ v6), we saw no performant way to support getting property names of a Realm.Object via the standard Object.keys(obj). As a side-effect we stopped supporting the object spread operator `{...obj}` and `Realm.Object#keys()`, `Realm.Object#entries()` and `Realm.Object#toJSON()` methods were introduced as a workaround. The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}` and therefore we suggest deprecating the API with the @deprecation (plus console.warn once) of RealmObject#keys() and RealmObject#entries(). RealmObject#toJSON still serves the purpose of producing a circularly referencing object graph. We would love the community's feedback on this!
* The [push service is deprecated](https://www.mongodb.com/docs/atlas/app-services/reference/push-notifications/) from the servers point of view, we've deprecated this on v11 and removed it from v12.
* We’ve decided to remove numeric indexing and “array methods” from the SubscriptionSet, since it was would bloat our SDK code, the team see little actual use-case for it, we expect very few developers actually using this and the workaround is really easy (spreading into an array `[...subscriptions]`). Again something we would love feedback on.
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally, not expected to be used by users and most are very simple to type out for any user relying on it. Similarly the `DictionaryBase` is a type that was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident. We propose removing this and ask users to simply use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) on the `Realm.Sync.Session`. We’re no longer using the `*Payload` types (were actually only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to this suggestion, I think it would be good to clarify whether or not DictionaryBase has been removed (it wasn't all that clear to me if it has).

Suggested change
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally, not expected to be used by users and most are very simple to type out for any user relying on it. Similarly the `DictionaryBase` is a type that was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident. We propose removing this and ask users to simply use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) on the `Realm.Sync.Session`. We’re no longer using the `*Payload` types (were actually only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.
* No longer exporting the `ObjectPropsType`, `UserMap`, `UserType`, `BaseFunctionsFactory`, `AuthProviders`, `PropertyType`, `HTTP`, `*Details` interfaces of the `EmailPasswordAuthClient` and `AuthError` types, since it wasn’t used internally and not expected to be used by users. Moreover, most are very simple to type out for any user relying on it. Similarly, the `DictionaryBase` type was introduced to help work around an issue (declaring string index accessors on a class with methods) in our declarations. We consider it an internal detail that got introduced as part of our public API by accident; thus, we ask users to use the `Dictionary` type directly. We also decided to rename the `Session` class to `SyncSession` since it’s now exported directly on the package namespace. `Session` will still be available (but deprecated) on the `Realm.Sync.Session`. We’re no longer using the `*Payload` types (which were only used by Realm Web) and we don’t expect end-users to be relying directly on these, hence they will be deleted.

CHANGELOG.md Outdated

### Enhancements
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* The new SDK wraps its accessor objects in a Proxy trapping the ownKeys trap which enables calls to the standard `Object.keys(obj)` and the spread operator `{...obj}`
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
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
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. If set to true, all async operations (such as opening the Realm using `Realm.open()`) will fail when a non-fatal error, such as a timeout, occurs.

@@ -1,6 +1,6 @@
{
"name": "realm",
"version": "12.0.0-rc.0",
"version": "12.0.0-alpha.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

🥳

@kraenhansen
Copy link
Member

Thanks y'all for the super fast replies! I incorporated most of your feedback and I can't wait to push that button first thing tomorrow 😄

@github-actions github-actions bot merged commit af12c2e into main Mar 22, 2023
@github-actions github-actions bot deleted the release/12.0.0-alpha.0 branch March 22, 2023 08:32
papafe added a commit that referenced this pull request Mar 22, 2023
* main:
  Prepare for vNext
  Fixed extraction of version in publish workflow
  Prepare for 12.0.0-alpha.0 (#5600)
  Remove redundant MongoDBCollection watch tests.

# Conflicts:
#	CHANGELOG.md
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 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.

7 participants