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

Node and React Native Data Types #1065

Merged
merged 29 commits into from
May 19, 2021
Merged

Conversation

mohammadhunan-dev
Copy link
Contributor

@mohammadhunan-dev mohammadhunan-dev commented May 11, 2021

Note to the reviewer

  • This PR only needs a sanity check review, as the pages of this PR have already been copy and technical reviewed in other PRs.

Pull Request Info

Jira

Staged Changes (Requires MongoDB Corp SSO)

Review Guidelines

REVIEWING.md

Mohammad Hunan Chughtai and others added 9 commits May 5, 2021 20:55
* new data types for node

* added data types to TOC

Co-authored-by: Mohammad Hunan Chughtai <[email protected]>
* Added node.js field types

* fixed wording

* fixed typo

* fix typo in mdn urls

* added additional data types

* fix monospace err

Co-authored-by: Mohammad Hunan Chughtai <[email protected]>
* Add content to Node.js > Data Types > Collections.txt

* Removed unneeded spacing

* clean up collections page + add headers

* Filled out collections page

* fix refs

* fix typo

* Update source/sdk/node/data-types/collections.txt

* Update source/sdk/node/data-types/collections.txt

* Update source/sdk/node/data-types/collections.txt

Co-authored-by: Dachary <[email protected]>

* fix rst

* fix woridng

* added ref

* removed unneeded word

* removed unneeded word

* added period

Co-authored-by: Mohammad Hunan Chughtai <[email protected]>
Co-authored-by: Dachary <[email protected]>
* Added documentation of Node.js embedded objects under data types

* clean up relationships and embedded objects

* added CRUD examples for embedded obj

* added bluehawked examples

* fix rst syntax highlight + add description for deletes

* Update source/sdk/node/data-types/embedded-objects.txt

* Update source/sdk/node/data-types/embedded-objects.txt

* Update source/sdk/node/data-types/embedded-objects.txt

* Update source/sdk/node/data-types/embedded-objects.txt

* Update source/sdk/node/data-types/embedded-objects.txt

Co-authored-by: Mohammad Hunan Chughtai <[email protected]>
* attempt to add bluehawked mixed example snippets

* fixed wording

* Update source/examples/generated/node/data-types.codeblock.query-objects-with-mixed-values.js

* Update examples/node/Examples/data-types.js

Co-authored-by: Mohammad Hunan Chughtai <[email protected]>
Mohammad Hunan Chughtai and others added 3 commits May 13, 2021 01:50
* (DOCSP-14565): Dictionary Data Type - Node.js

* added unit tested + bluehawked dictionary examples

* Update examples/node/package.json

* removed unneeded file

* Added new examples for dictionaries

* update dictionary examples

* fix capitalization

* moved addlistener down

* fix comment

* update wording to be clearer on type usage in dict

* fix wording

* fix wording

Co-authored-by: Mohammad Hunan Chughtai <[email protected]>
* bump realmjs to 10.5.0-beta-1

* removed uuid as it's own page and added a subsection on it

* added uuid bluehawked snippet + readded uuid to toc

* added uuid examples

* removed uuid from field types as a paragraph

* update wording

* update wording

* fix passive voice

* add specific uuid example

* removed innacurate collections are homogenous (per mixed)
* added set examples + bluehawked

* literal included set exampkes

* added delete one set item example

* fix typo

* added descriptions to set

* fix grammar

* changed link + hunter to generic names

* added capitalization to clearly define Realm Set as a term

* fix wording + formatting"

* clarified wording

* fix literalincldue indentation

* fix typo

* more wording fixes

* clean up realm object model for set wording

* fix character names for examples

* clarified traversal order wording

* rst typo in <set>

* changed set to mySet

* fix overview wording

* updated overview to be more clear on differences between array

* updated create wording

* Update source/examples/generated/node/data-types.codeblock.remove-all-items-from-set.js

Co-authored-by: Kenneth Geisshirt <[email protected]>

* Update source/examples/generated/node/data-types.codeblock.remove-specific-item-from-set.js

Co-authored-by: Kenneth Geisshirt <[email protected]>

* Update source/sdk/node/data-types/sets.txt

Co-authored-by: Kenneth Geisshirt <[email protected]>

* fix grammar + wording + changed Set to Realm.Set in js snippets

* Fix woridng

Co-authored-by: Kenneth Geisshirt <[email protected]>
@mohammadhunan-dev mohammadhunan-dev changed the title WIP - Node Data Types Node Data Types May 19, 2021
@mohammadhunan-dev mohammadhunan-dev changed the title Node Data Types Node and React Native Data Types May 19, 2021
Copy link
Contributor

@nathan-contino-mongo nathan-contino-mongo left a comment

Choose a reason for hiding this comment

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

LGTM with some nitpicks and suggestions on the Node docs. All of my comments apply twofold: once to the Node page I commented on, but also to the React Native equivalent of the page that just has different anchor link names.

- ``objectId`` maps to BSON :manual:`ObjectId </reference/method/ObjectId/>` type.
- ``data`` maps to the JavaScript :mdn:`ArrayBuffer <Web/JavaScript/Reference/Global_Objects/ArrayBuffer>` type.
- ``date`` maps to the JavaScript :mdn:`Date <Web/JavaScript/Reference/Global_Objects/Date>` type.
- ``list`` maps to the JavaScript :mdn:`Array <Web/JavaScript/Reference/Global_Objects/Array>` type. You can also specify that a field contains a list of a primitive value type by appending ``[]`` to the type name.
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
- ``list`` maps to the JavaScript :mdn:`Array <Web/JavaScript/Reference/Global_Objects/Array>` type. You can also specify that a field contains a list of a primitive value type by appending ``[]`` to the type name.
- ``list`` maps to the JavaScript :mdn:`Array <Web/JavaScript/Reference/Global_Objects/Array>` type. You can also specify that a field contains a list of a primitive value types by appending ``[]`` to the type name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed locally 👍 , ignoring the commit suggestion. Changed it to "...contains a list of primitive value types by appending []"


Overview
--------
A ``{+service-short+} Set`` is a special object that allows you to store a
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the SDK actually uses the term Realm.Set (with a period in the middle) to refer to this type. Since "Realm Set" (with a space, no period) isn't actually a type in the SDK, I would suggest either:

  • removing the monospace formatting from the term (reserved for literal type names)
  • switching the space to a period so we're using the literal name of the type in the SDK.

If the SDK uses the name "Realm Set" with a space in the middle, then you can safely ignore this. But since most languages don't support type names containing spaces, I think we ought to change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SDK term is Realm.Set, but I've defined it as "Realm Set" to indicate that it the term is a proper noun and to avoid confusion with the JavaScript set term.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but you shouldn't monospace the term if it isn't a literal code term. You can bold it if you want to highlight it as a key term, but monospace is reserved for code references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, didn't realize monospace was reserved for code references. Changing it to bold now.

Comment on lines 21 to 30
- bool
- int
- float
- double
- string
- Date
- Data
- UUID
- Set
- null
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are all literal type names, can we monospace format these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 👍

-------------------
To :ref:`set a property of your object model
<node-define-a-realm-object-schema>` as mixed, set the property's type to
``"mixed"``.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: can we move the quotes here outside of the monospace formatting, since it's not part of the literal type name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 👍


The mixed data type is indexable, but you can't use it as a primary key.
Because null is a permitted value, you can't declare a Mixed property as
optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is a bit confusing to me -- the second bit doesn't really follow from the first. If a Mixed property can have the value null, wouldn't it essentially always be optional? Perhaps a second clarifying sentence to explain this could be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optional means that it can or cannot contain a value, and null is technically a value. Going to avoid changing this for now, since this sentence has already been copy + tech reviewed, but will add the suggestion to a list of iterative updates.

source/sdk/node/data-types/uuid.txt Outdated Show resolved Hide resolved
In general, you can use ``UUID`` in any field that you need a unique
identifier. Using ``UUID`` might be particularly useful if you are migrating
data not stored in MongoDB since it is likely that your object's unique
identifiers are already of a ``UUID`` type.
Copy link
Contributor

Choose a reason for hiding this comment

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

No callout here to the fact that you should use ObjectId for collections of data that already exist in MongoDB? That feels like a significant omission since without that statement we basically never talk about when you would actually use ObjectId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I've added the sentence "Alternatively, using ObjectId might be useful for a collection of data that already exists in MongoDB."

source/sdk/node/data-types/mixed.txt Outdated Show resolved Hide resolved
Comment on lines +34 to +35
your :ref:`object model <node-object-schemas>`. Create a {+service-short+}
object within a write transaction. To set any unique identifier properties of
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
your :ref:`object model <node-object-schemas>`. Create a {+service-short+}
object within a write transaction. To set any unique identifier properties of
your :ref:`object model <node-object-schemas>`. To set any unique identifier properties of

I'm not sure if the command/instruction to "create a Realm object within a write transaction" actually fits here. Readers already know that you're talking about a theoretical object model that contains a UUID field, so personally I'd say that the instructions to set properties with new UUID() should suffice to explain how to generate new UUID instances and put them in such an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to ignore this one, as it doesn't hurt to re-explain that it needs to be within a write transaction. Personally, I've made the mistake of not writing in a write transaction, and then realized once I got the error, so I think a short reminder sentence might be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, but just note that we don't tell users to modify fields in write transactions in the very same paragraph, even though you need to use a write transaction for all modifications.

Perhaps you've missed the point of my comment, though, which was that instructing users to create an instance of the object is probably redundant since they'll know that they need an instance to modify a property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'in the very same paragraph' as in the last sentence of that paragraph?: 'Alternatively, set a string to new UUID() to set the unique identifier property to a specific value.'? That sentence is meant to show a specific usage of the new UUID constructor, rather than how to use UUIDs as a whole within the realm and within a write object (which the previous sentences teach).

Even if it's a bit redundant, I think that sentence still works and could be helpful to readers who may not know or who forget that you need a write transaction and an instance of an object to set or modify a uuid property.

Let me know if I'm misunderstanding your concern/suggestion though.

@mohammadhunan-dev mohammadhunan-dev merged commit f43ae61 into master May 19, 2021
@mohammadhunan-dev mohammadhunan-dev deleted the new-data-types-node-rn branch May 19, 2021 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants