Fix a slew of Node.js tests and regenerate some snippets #3352

krollins-mdb · 2024-07-31T20:23:03Z

Pull Request Info - SDK Docs Consolidation

This PR mostly fixes fixes tests, but made some snippet changes for a couple of pages too.

Release Notes

Mostly internal: Node SDK
- Fixed failing tests

- update counters tests

- Change schema property id to object id - Fix find predicate - Add JS quickstart test - Update quickstart to use new generated snippets

netlify · 2024-07-31T20:23:20Z

✅ Deploy Preview for device-sdk ready!

Name	Link
🔨 Latest commit	`d569cb8`
🔍 Latest deploy log	https://app.netlify.com/sites/device-sdk/deploys/66b244e4c4865f000854aabb
😎 Deploy Preview	https://deploy-preview-3352--device-sdk.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 32 (🔴 down 1 from production) Accessibility: 93 (no change from production) Best Practices: 75 (no change from production) SEO: 69 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions · 2024-07-31T20:24:20Z

Readability for Commit Hash: d569cb8

You can see any previous Readability scores (if they exist) by looking
at the comment's history.

Readability scores for changed documents:

source/sdk/node/quick-start: Grade Level: 8.8, Reading Ease: 51.85
source/sdk/node/model-data/data-types/counters: Grade Level: 8.2, Reading Ease: 58.79

For Grade Level, aim for 8 or below.

For Reading Ease scores, aim for 60 or above:

Score	Difficulty
90-100	Very Easy
80-89	Easy
70-79	Fairly Easy
60-69	Medium
50-59	Fairly Hard
30-49	Hard
0-29	Very Hard

For help improving readability, try Hemingway App.

dacharyc

Handful of Qs here, but none of them are blocking. Thanks for fixing up these tests!

dacharyc · 2024-08-05T17:54:46Z

examples/node/v12/__tests__/models/models.js

+import Realm, { BSON } from "realm";
+
+// :snippet-start: declare-counter-schema
+export class ClassWithCounter extends Realm.Object {


This is a nitpick and I know we haven't had a chance to discuss code example style stuff, but I really prefer to avoid arbitrary names like ClassWithCounter. I'd rather we used an actual real-world use case name and example. I don't want there to be any misunderstanding about whether there is required usage stuff encoded in our naming in the code examples. (I.e. someone seeing something called ClassWithCounter or counterProperty and thinking the naming somehow has to match that in order to use this functionality.)

So maybe this could be something like SiteVisitTracker or something where a counter would make sense logically, but doesn't directly tie in with the naming of the thing?

Yeah, I agree. I was struggling with determining how much to change the stuff Omar added. I think I ended up in a weird middle ground. I'll update the class and property names.

dacharyc · 2024-08-05T17:56:27Z

examples/node/v12/__tests__/models/models.js

+// :snippet-end:
+
+// :snippet-start: define-an-object-model
+export class QuickstartTask extends Realm.Object {


Presumably the export here is related to the fact that we're including these models here in this file, but using them somewhere else in the code.

Just wondering if we actually want to show this as part of our class declarations, or should we declare the class here and then export it later outside of this snippet?

Generally, yes. Best practice with the JS SDK is to define models in a separate file from where you use them. So, the default should export/import.

Worth questioning here though. In the Reactive Native docs, the standard is pretty well established. I don't think as clear in the Node docs. I'll think about this a bit more.

dacharyc · 2024-08-05T17:59:14Z

examples/node/v12/__tests__/quickstart.test.js

+describe("Quickstart Sync", () => {
+  test("should open a Flexible Sync realm with initial subscriptions", async () => {
+    // :snippet-start: open-realm-with-subscriptions
+    // :replace-start: {


Hmm, this is probably another thing we should discuss as part of our code example style convo. In other SDKs, we've had the convention of declaring the "replace" block at the start of the file. I would not have expected to find one in the middle of the file, and so if I didn't see it at the top I would have assumed there wasn't one in this file.

We can bikeshed over which is better, but I think it would be good to agree to a convention as a team and follow it, so we can have consistent expectations about where to find things in each SDK's test suite.

Happy to standardize - though I don't think any of the JS docs have ever done that. 😅

dacharyc · 2024-08-05T18:04:10Z

examples/node/v12/__tests__/quickstart.test.js

+      .objects(QuickstartTask)
+      .filtered(`owner_id == "${anonymousUser.id}"`);
+
+    expect(assignedTasks.length).toBe(0);


In some of the other sync quick starts, we actually write stuff, verify it uploaded, and then delete it.

Should we do that in this test?

Probably. If I don't have time to do it in this PR, I'll create a Jira ticket so that the work is documented.

dacharyc · 2024-08-05T18:10:19Z

source/sdk/node/model-data/data-types/counters.txt

-
-      .. literalinclude:: /examples/generated/node/data-types.test.snippet.initialize-counter.js
-          :language: javascript
+.. include:: /examples/generated/node/v12/formatted/data-types.test.snippet.initialize-counter.ts.rst


We may want to discuss how to handle these things going forward, but for the consolidated world, seeing only a .ts file here would make me say the JS examples are missing. I would put in a "missing placeholder" for the JS.

If we choose not to maintain two separate snippets because the code would be the same in JS and TS, can we use some sort of naming convention to make it clear to folks who don't regularly do JS and TS dev stuff that this snippet works for both?

Good point. Yeah, we'll need something. JS tests should be TypeScript first, even when there isn't any TypeScript code. The reasoning being that the compiler can catch all sorts of errors with plain ol' JS.

I'll do some thinking on this one too.

docs-builder-bot · 2024-08-06T15:53:30Z

✨ Staging URL: https://preview-mongodbmongodb.gatsbyjs.io/realm/master/

🪵 Logs

job log: be7913a934b47546cd39205f4ffb95089120c750

krollins-mdb added 5 commits July 31, 2024 13:43

- update dependencies

138b962

- update counters tests

Fix existing object throwing off expected value

f17c021

- Fix another existing object issue

f58829b

- Move TS type bug exception

f49210a

- Change schema property id to object id - Fix find predicate - Add JS quickstart test - Update quickstart to use new generated snippets

Add ts-ignore for old API

c74d223

krollins-mdb added the Add to consolidation feature branch PR merged after cutting the consolidation feature branch, so cherry-pick to the feature branch. label Jul 31, 2024

krollins-mdb added 2 commits July 31, 2024 15:31

Minor cleanup

21acc63

Fix bad include ref

2dae749

krollins-mdb marked this pull request as ready for review July 31, 2024 20:41

dacharyc approved these changes Aug 5, 2024

View reviewed changes

Implement feedback

d569cb8

krollins-mdb merged commit be7913a into mongodb:master Aug 6, 2024
12 checks passed

krollins-mdb deleted the node-tests-exploration branch August 6, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a slew of Node.js tests and regenerate some snippets #3352

Fix a slew of Node.js tests and regenerate some snippets #3352

krollins-mdb commented Jul 31, 2024 •

edited

Loading

netlify bot commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

dacharyc left a comment

dacharyc Aug 5, 2024

krollins-mdb Aug 5, 2024

dacharyc Aug 5, 2024

krollins-mdb Aug 5, 2024

dacharyc Aug 5, 2024

krollins-mdb Aug 5, 2024

dacharyc Aug 5, 2024

krollins-mdb Aug 5, 2024

dacharyc Aug 5, 2024

krollins-mdb Aug 5, 2024

docs-builder-bot commented Aug 6, 2024

Fix a slew of Node.js tests and regenerate some snippets #3352

Fix a slew of Node.js tests and regenerate some snippets #3352

Conversation

krollins-mdb commented Jul 31, 2024 • edited Loading

Pull Request Info - SDK Docs Consolidation

Release Notes

netlify bot commented Jul 31, 2024 • edited Loading

✅ Deploy Preview for device-sdk ready!

github-actions bot commented Jul 31, 2024 • edited Loading

dacharyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docs-builder-bot commented Aug 6, 2024

🪵 Logs

krollins-mdb commented Jul 31, 2024 •

edited

Loading

netlify bot commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading