refactor(NODE-6055): implement OnDemandDocument #4061
Conversation
commit 7768782191e140b71dc33183fa6052dc4667a11a Author: Neal Beeken <[email protected]> Date: Thu Mar 28 10:28:55 2024 -0400 chore(NODE-XXXX): add getValue method to OnDemandDocument
package.json
Outdated
| @@ -26,7 +26,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@mongodb-js/saslprep": "^1.1.5", | |||
| "bson": "^6.5.0", | |||
| "bson": "github:mongodb/js-bson#main", | |||
There was a problem hiding this comment.
As part of this review we can determine if we want anything moved to BSON, and I can fix the "this" issue with the NumberUtils, and we can put out a release and update this PR before merging it.
nbbeeken
changed the title
nbbeeken
added
the
Primary Review
| /** Caches the existence of a property */ | ||
| private readonly existenceOf: Record<string, boolean> = Object.create(null); | ||
| /** Caches a look up of name to element */ | ||
| private readonly elementOf: Record<string, BSONElement> = Object.create(null); |
There was a problem hiding this comment.
isn't existenceOf[name] basically the same as elementOf[name] != null? do we need existenceOf?
There was a problem hiding this comment.
Not quite, if I look up a key that doesn't exist, I won't know of its non-existence until I get to the end of all the elements. Without this cache non-existence will look it up again.
src/utils.ts
Outdated
| @@ -1301,18 +1301,18 @@ export async function once<T>(ee: EventEmitter, name: string): Promise<T> { | |||
| } | |||
| } | |||
|
|
|||
| export function maybeAddIdToDocuments( | |||
| coll: Collection, | |||
| export function maybeAddIdToDocuments<T extends Document>( | |||
There was a problem hiding this comment.
Is this related to the work in this PR or just a nice improvement?
There was a problem hiding this comment.
Accidentally left over from the PoC, we may want this at some point, but we can do it when necessary.
| private readonly elements: BSONElement[]; | ||
|
|
||
| /** The number of elements in the BSON document */ | ||
| public readonly length: number; |
There was a problem hiding this comment.
I'd still lean toward size() here, but less is more, we can revist this if it is needed. For example, cursor might want to know how many docs are left but we should solve for that case.
|
mongodb/js-bson#665 Has BSON clean-up changes needed to get this branch off a git hash. |
nbbeeken
changed the title
Co-authored-by: Bailey Pearson <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
Description
What is changing?
includesbyte comparison methodIs there new documentation needed for these changes?
No
What is the motivation for this change?
OnDemandDocument will provide the foundation for doing less up front parsing work.
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript