refactor!: format sort in cursor and in sort builder - #2573
Conversation
| } | ||
|
|
||
| // Get the next available document from the cursor, returns null if no more documents are available. | ||
| export function nextObject(cursor: Cursor, callback: Callback): void { |
| } | ||
|
|
||
| /** converts a Sort type into a type that is valid for the server (SortForCmd) */ | ||
| export const formatSort = SortDigest.prepare; |
There was a problem hiding this comment.
Thought it would be good to keep this as a black box.
There was a problem hiding this comment.
Makes sense! since we just export prepare, maybe we should name it formatSort this goes along with my comment of pulling the functions to the top level of the module, my thinking its a little more ctrl-F friendly this way.
| var cursor = collection.find().sort(['a', 1]); | ||
| test.deepEqual(['a', 1], cursor.sortValue); | ||
| finished(); | ||
| test.deepEqual({ a: 1 }, cursor.sortValue); |
There was a problem hiding this comment.
these had to change, it no longer just places the value it formats & validates it.
| }); | ||
| cursor = collection.find(); | ||
| try { | ||
| cursor.sort(25); |
There was a problem hiding this comment.
This now throws a sync error, which is a breaking change?
There was a problem hiding this comment.
I think this is fine, but we should probably add a ! to the commit and make a note that sort direction is now being validated more strictly.
cb3c039 to
2f4131d
Compare
nbbeeken
left a comment
nbbeeken
left a comment
There was a problem hiding this comment.
Great start, a few comments
| type SortForCmd = { [key: string]: SortDirectionForCmd }; | ||
|
|
||
| /** @internal */ | ||
| class SortDigest { |
There was a problem hiding this comment.
Since all the methods are static so whats the motivation for making this a class?
There was a problem hiding this comment.
I prefer to house the utilities used as static methods on a class, but they could be flat functions in this file.
There was a problem hiding this comment.
Since we haven't done it elsewhere I think its best to leave them as top level functions, the fact that they live in their own file I think already provides the boxing you're looking for.
| return t.reduce((acq, i) => { | ||
| return { ...acq, ...SortDigest.pairToObject(i) }; | ||
| }, {}); |
There was a problem hiding this comment.
It's unlikely that anyone will have large sort specifiers but nonetheless this nested looping feels like something to avoid despite the potentially small input.
There was a problem hiding this comment.
I'm confused, this seems essential, how else would you convert [['a', 1], ['b', 'desc']] to {a: 1, b: -1}?
There was a problem hiding this comment.
Wouldn't this be equivalent without the inner loop?
let sortObject = {};
for (const [name, value] of t) {
sortObject[name] = prepareDirection(value);
}
return sortObject| if (Array.isArray(sort) && !sort.length) return undefined; | ||
| if (typeof sort === 'object' && !Object.keys(sort).length) return undefined; |
There was a problem hiding this comment.
[1000% nit]: I would use x.length !== 0 but that is a total 🧅
There was a problem hiding this comment.
How would this proposal work?
({ 'neal': true }).length=> undefinedObject.keys({ 'neal': true }).length=> 1
There was a problem hiding this comment.
I should clarify I meant in addition to what you have.
if you already check Array.isArray(sort) is true then sort will have a length prop that is a number.
Object.keys(sort) returns an array so you can check the length === 0 on that too.
There was a problem hiding this comment.
Hmmm, I'm 100% down to do this, not sure I'm fully understanding, can you give me a suggestion using the github feature / code sample?
3 participants
NODE-2458
This PR attempts to disambiguate what the difference between the
Sortthat the user enters in, and the$sortthat is sent to the server, and to format accordingly. Even though acmdis any / document, that's not really true, it still takes specified key / value types, and it's not necessary apparent what the difference is.