-
Notifications
You must be signed in to change notification settings - Fork 88
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
DOCSP-29928 Serialization for Dart #3235
Conversation
Readability for Commit Hash: d7de333 You can see any previous Readability scores (if they exist) by looking Readability scores for changed documents:
For Grade Level, aim for 8 or below. For Reading Ease scores, aim for 60 or above:
For help improving readability, try Hemingway App. |
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 have added a few comments.
I also noticed that the syntax highlighting is missing a few places.
My biggest question is; if we want to show how to serialize non-realm objects? I guess we can wait with that until later, perhaps when the new mongo db client (or what it ends up being named lands).
Thanks @nielsenko! Let me know if there is anything else. Will look into why the syntax highlighting is off. For serializing non-realm objects, I linked to this page on BSON Data Types and Representation in the "Supported Data Types" section of the page. Edit: Unfortunately, we don't have control over whether the syntax highlighting registers or not. |
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.
Back to you with some questions and comments. And let's clean up and refactor the tests for idempotency
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.
Some suggestions but overall I think this looks good
source/examples/generated/flutter/serialization_test.snippet.serialize.dart
Outdated
Show resolved
Hide resolved
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.
LGTM. I have a few suggestions, but feel free to disregard them if you don't agree.
- Array | ||
- Binary | ||
- Boolean | ||
- Date | ||
- Document | ||
- Decimal128 | ||
- Double | ||
- Int32 | ||
- Int64 | ||
- MaxKey | ||
- MinKey | ||
- ObjectID | ||
- String | ||
- Symbol | ||
- Null | ||
- Undefined |
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.
Consider annotating each entry with the associated dart type perhaps, ie. Array (List)
, Binary (Uint8List)
, Document (Map<String, dynamic>)
, etc.
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.
Agreed. Can we make this a table, @lindseymoore? Alternately, we could make it a table of:
Dart/Realm type | Serialized type | Example output
- Undefined | ||
|
||
For more information and examples on the serialization for each of these types, | ||
see :manual:`BSON Data Types and Associated Representations </reference/mongodb-extended-json/#bson-data-types-and-associated-representations>`. |
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.
Following this link, and then the link to the full spec, I noticed it is redirecting us to https://github.com/mongodb/specifications/blob/master/source/extended-json.md. Completely unrelated to this PR, but perhaps worth looking into.
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.
Formatting and content suggestions. We also need to make sure the other affected Flutter pages are updated with this new method (e.g. Call an Atlas Function)
|
||
The Atlas Device SDK for Flutter supports serialization and deserialization of | ||
:manual:`Extended JSON (ESJON) </reference/mongodb-extended-json>` to and from static Realm objects. | ||
Serialization allows for easier integration with MongoDB APIs. |
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.
Serialization allows for easier integration with MongoDB APIs. | |
The Flutter SDK can use this full-document EJSON encoding to communicate | |
directly with MongoDB Atlas, such as through the following APIs: | |
- App Services :ref:`Function calls <flutter-call-function>` | |
- Credentials with :ref:`custom function authentication <flutter-login-custom-function>` | |
- User profile and :ref:`custom user data <flutter-custom-user-data>` |
You'll also need to check these linked pages and make sure there aren't any doc impacts (e.g. the Call a Function page instructions need to be updated with this new ESJON method)
fyi - this list will eventually include the enhanced Query MongoDB functionality, which hasn't been released yet
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.
Ticket for this work: https://jira.mongodb.org/browse/DOCSP-41296
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 think the suggested edits here are still valid, even if the linked pages will be updated in separate tickets (let's make sure we have tickets tracking the custom function auth updates too. doesn't look like custom user data has any impacts)
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 see the function on this page converts to json rather than ejson. Is converting to ejson the more updated way; will it no longer work with json?
I'll add the function auth update to the previous ticket!
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.
ooh, good catch! 🕵️ looks like that call uses jsonEncode()
and jsonDecode()
internally, so we should probably still point users to serialize object args that way. Although now I'm a little confused by the last sentence... ("The Function returns a dynamic value containing MongoDB Extended JSON (EJSON) deserialized to a native Dart object.")
Either way, I think we're good to leave this as-is. Thanks for digging into this and confirming!
❌ Deploy Preview for device-sdk failed. Why did it fail? →
|
// Null | ||
int? a; | ||
|
||
// RealmValue supported??? Engineer said so but doesn't seem to be so |
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.
- Can we update these comments to be more descriptive of the issue (to help anyone else reading them who might not have the context)?
- Do we have a ticket or something to track this work, so we can update once engineering get the bug fixes in? @krollins-mdb do we have a standard for handling feature docs with known bugs? i.e. do we need to note anything in the docs?
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.
Added the engineering ticket to a comment (RDART-1063). I can make a DOCSP ticket to update this section with RealmValue and Decimal128 once engineering ticket is resolved. Doesn't seem to be high priority for engineering, so I'm comfortable listing RealmValue and Decimal128 as unsupported for now. Will let engineering know if that sounds good!
DOCSP ticket: https://jira.mongodb.org/browse/DOCSP-41612
|
||
The Atlas Device SDK for Flutter supports serialization and deserialization of | ||
:manual:`Extended JSON (ESJON) </reference/mongodb-extended-json>` to and from static Realm objects. | ||
Serialization allows for easier integration with MongoDB APIs. |
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 think the suggested edits here are still valid, even if the linked pages will be updated in separate tickets (let's make sure we have tickets tracking the custom function auth updates too. doesn't look like custom user data has any impacts)
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.
🚀 You did it! Page looks great, @lindseymoore! ❤️
✨ Staging URL: https://preview-mongodbmongodb.gatsbyjs.io/realm/master/ 🪵 Logs |
Pull Request Info
Jira ticket: https://jira.mongodb.org/browse/DOCSP-29928
Reminder Checklist
Before merging your PR, make sure to check a few things.
Release Notes
Flutter SDK
Review Guidelines
REVIEWING.md