Conversation
| // @ts-expect-error Write concern is disallowed in types, but | ||
| // must still be removed in case the user is not using Typescript | ||
| delete this.options.writeConcern; |
There was a problem hiding this comment.
Interesting that we need ts-expect-error in src code, is there another way we could organize this? (can be in a follow up)
There was a problem hiding this comment.
Not with strong typing. I mean, ultimately the real fix is to pass the full operation all the way down to the command layer, so that when we apply the session, we have access to the operation class and can check if it has a read aspect.
There was a problem hiding this comment.
If we're creating a new object, then can their be a new type that allows deleting the writeConcern without a ts-expect-error?
There was a problem hiding this comment.
Do you mean like an internal options type? Does that not seem like overkill?
There was a problem hiding this comment.
Also, how / why would we type something such that it allows the existence of a property that we will always delete?
There was a problem hiding this comment.
What do you mean by types not being true? The type is correct. The argument could be made that we should omit this logic entirely because if someone passes a write concern to read operation and our types no longer support it, they are operating outside the bounds of our API.
There was a problem hiding this comment.
I thought the issue was with the option being inherited rather than being directly passed, right?
I think if it was a correct type then we wouldn't need ts-expect-error
There was a problem hiding this comment.
Good point about options being inherited.
I don't think the value of modifying the type to include writeConcern?: never is worthwhile. I don't think that's the correct fix (I mention what I believe the correct fix to be above), so I'm going to leave this as-is.
There was a problem hiding this comment.
Alternative option, again outside the scope of this work, is to consider a generic approach to constructing the options objects we use internally. Each command would construct an options object, only assigning the options that are actually supported for that command, instead of spreading all the existing options (maybe add an abstract parseOptions(userOptions, inheritedOptions) : OptionInterface method that each command implements). But that is a larger change that impacts the driver as a whole, so I won't address it here.
There was a problem hiding this comment.
I seem to recall a ticket related to that idea was a part of the Operation Layer Unification epic we used to have but I can't seem to find it, seems worth filing something for esp if we've considered it before.
I'd prefer if we made the types accurate rather than putting in compiler overrides
nbbeeken
left a comment
There was a problem hiding this comment.
requested changes already ^ bump
Description
What is changing?
Cherry-picked changes from the 4.x branch with the same fix.
Is there new documentation needed for these changes?
no.
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript