-
Notifications
You must be signed in to change notification settings - Fork 563
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
RJS-1413: Add support for a logical counter as a presentation data type #6694
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a bit more docs onto this Counter
class as we don't have it yet for the MDB docs. Please see if you think something should be reformulated/added/removed, etc.
/** @internal */ | ||
constructor(realm: Realm, obj: binding.Obj, columnKey: binding.ColKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. Despite @internal
, the constructors still show up in the API ref docs (for other classes as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit weird 🤔 Perhaps there's (still) something misconfigured in the way we generate docs then.
* If an existing object exists, only properties where the value has actually | ||
* changed will be updated. This improves notifications and server side | ||
* performance but also have implications for how changes across devices are | ||
* merged. For most use cases, the behavior will match the intuitive behavior | ||
* of how changes should be merged, but if updating an entire object is | ||
* considered an atomic operation, this mode should not be used. | ||
* If an existing object exists (determined by a matching primary key), only | ||
* properties where the value has actually changed will be updated. This improves | ||
* notifications and server side performance but also have implications for how | ||
* changes across devices are merged. For most use cases, the behavior will match | ||
* the intuitive behavior of how changes should be merged, but if updating an | ||
* entire object is considered an atomic operation, this mode should not be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only change here is adding (determined by a matching primary key)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cred @nirinchev
|
||
/* eslint-disable-next-line @typescript-eslint/no-explicit-any -- We define these once to avoid using "any" through the code */ | ||
export type AnyCollection = Collection<any, any, any, any, any>; | ||
/* eslint-disable-next-line @typescript-eslint/no-explicit-any -- We define these once to avoid using "any" through the code */ | ||
export type AnyDictionary = Dictionary<any>; | ||
/* eslint-disable-next-line @typescript-eslint/no-explicit-any -- We define these once to avoid using "any" through the code */ | ||
export type AnyList = List<any>; | ||
/* eslint-disable-next-line @typescript-eslint/no-explicit-any -- We define these once to avoid using "any" through the code */ | ||
export type AnySet = RealmSet<any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also adding an unmanaged set (as an array), which I thought we had already done.
}, | ||
"Cannot use shorthand '[]' and '?' in 'type' or 'objectType' when defining property objects", | ||
); | ||
describe("'counter'", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant
"Cannot use shorthand '?' in 'type' or 'objectType' when defining property objects", | ||
); | ||
|
||
itThrowsWhenNormalizing( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant
"Cannot use shorthand 'counter' in 'type' or 'objectType' when defining property objects. To use presentation types such as 'counter', use the field 'presentation'", | ||
); | ||
|
||
itThrowsWhenNormalizing( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant
"Primary keys must always be indexed.", | ||
{ isPrimaryKey: true }, | ||
); | ||
describe("'counter' & collection combinations", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant
{ isPrimaryKey: true }, | ||
); | ||
|
||
itThrowsWhenNormalizing( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the the introduction of the "presentation" concept has added to the complexity of implementing Counters. The concept is interesting, and I am looking forward on how we can use it in the future.
RC release train will take off now 🚋 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. As we have discussed the PR before the RC, I don't have any further comments (except a suggestion to link to the documentation).
Let's get it merged and released!
For reference on lack of TS support, see: microsoft/TypeScript#2361
Co-authored-by: Kræn Hansen <kraen.hansen@mongodb.com>
Co-authored-by: elle-j <elle-j@users.noreply.github.com>
Co-authored-by: elle-j <elle-j@users.noreply.github.com>
Co-authored-by: Kenneth Geisshirt <kenneth.geisshirt@mongodb.com>
Co-authored-by: Kræn Hansen <kraen.hansen@mongodb.com>
a92fb04
to
d936655
Compare
What, How & Why?
A
Counter
class andcounter
presentation data type have been added, representing a logical counter.Note that
counter
types are not supported as:Mixed
valuesCounter.value
)Usage
Valid property schema:
Creating an object with a counter:
Updating the count:
Creating a counter from a previously
null
value:Notes
number
:valueOf
partly due to TypeScript not supporting it.Test overview
Click to expand
This closes #4089
☑️ ToDos