refactor: Remove generic options parameter from OperationBase #2593
Conversation
src/utils.ts
Outdated
| if (name === 'session') continue; | ||
| if (ArrayBuffer.isView(value)) continue; |
There was a problem hiding this comment.
This is pretty lame skip logic, but I'm thinking maybe deepFreezing is overkill anyway. thoughts?
There was a problem hiding this comment.
I think it would be useful to explain why those need to be skipped with comments. Is there a reason we can't use a popular public domain implementation of this like the deep-freeze npm module?
There was a problem hiding this comment.
I got the code from the same place that dep gets it from the MDN docs, I figure for the small changes we might want to make, might as well in house it.
I did document these skips but I think I can come up with better logic.
| // if (this.writeConcern && this.writeConcern.w === 0) { | ||
| // if (this.session && this.session.explicit) { | ||
| // throw new MongoError('Cannot have explicit session with unacknowledged writes'); | ||
| // } | ||
| // return; | ||
| // } |
There was a problem hiding this comment.
I ran into a test failure when trying to add a few more operations to this PR that relates to this in applySession which is commented saying it should occur at command construction, I'll still have to discover the bug, but shall we move this here?
Maybe as part of removing legacy operations so everything would go through this path.
|
|
||
| // Freeze properties before freezing self | ||
|
|
||
| for (const name of propNames) { |
There was a problem hiding this comment.
Concern here about the combination of using deepFreeze within getters, this for loop will run on every access this.thing, and it's pretty hefty for loop. We may be making the object immutable but it's not the same object every-time this getter is run. Thoughts on some memoization or caching functionality?
There was a problem hiding this comment.
This could easily be achieved by setting this.cacheBuildOptions from within the getter constructor, and return that if it exists.
May have to just create buildOptions in the constructor, and any builder methods overwrite it and create a new object.
There was a problem hiding this comment.
I think it's fine in terms of performance if we minimize use of the getter, I commented about that above. In terms of immutability, I don't think we can expect something that is gradually built on an object to be the same over the object's lifespan, so I think this is OK.
src/operations/command.ts
Outdated
| const serverWireVersion = maxWireVersion(server); | ||
| const inTransaction = this.session && this.session.inTransaction(); | ||
|
|
||
| if (this.readConcern && commandSupportsReadConcern(cmd) && !inTransaction) { | ||
| Object.assign(cmd, { readConcern: this.readConcern }); | ||
| } | ||
|
|
||
| if (options.collation && serverWireVersion < SUPPORTS_WRITE_CONCERN_AND_COLLATION) { | ||
| if (this.builtOptions.collation && serverWireVersion < SUPPORTS_WRITE_CONCERN_AND_COLLATION) { |
There was a problem hiding this comment.
Any reason you changed these options to this.builtOptions instead of changing above to const options = this.builtOptions? Seems like we're calling that deepFreeze getter repeatedly without need here.
There was a problem hiding this comment.
Good catch I'm going through and changing, in fact I think I want to make this a method now, leaving this much work in a property access its easy to overlook that its doing work each time.
|
|
||
| // Freeze properties before freezing self | ||
|
|
||
| for (const name of propNames) { |
There was a problem hiding this comment.
I think it's fine in terms of performance if we minimize use of the getter, I commented about that above. In terms of immutability, I don't think we can expect something that is gradually built on an object to be the same over the object's lifespan, so I think this is OK.
src/utils.ts
Outdated
| if (name === 'session') continue; | ||
| if (ArrayBuffer.isView(value)) continue; |
There was a problem hiding this comment.
I think it would be useful to explain why those need to be skipped with comments. Is there a reason we can't use a popular public domain implementation of this like the deep-freeze npm module?
nbbeeken
force-pushed
the
NODE-2815/remove-generic-opteraion-base
branch
from
4b184ab to
3fe7652
Compare
Add a builtOptions getter that inherits options from the parent class. NODE-2815
nbbeeken
force-pushed
the
NODE-2815/remove-generic-opteraion-base
branch
from
3fe7652 to
948725a
Compare
|
Superseded by #2659 |
Add a
builtOptionsgetter that inherits options from the parent class.NODE-2815
Description
In order to make tracing options sensible I went with this idea of inheriting the super class's
builtOptionswhich are frozen. I'll put this small start in review to get some feedback going on how we like this methodology and I can start expanding this to other operations.Some of the bsonOptions work is duplicated in here to get the code to work, but I will pull that in from master when its there.
Look out for a parallel PR to this that works on removing executeLegacyOperation.