fix: remove shallow cloning of object when executing a batch http req… #679
Conversation
|
@bennyhobart: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
|
Having read the discussion in #461, this seems reasonable to me, because the current behavior is confusing and blocks optimizations we hope to get from batched queries (e.g. when sharing data loaders). It is likely some people rely on the context being shallowly cloned however, so this change will be breaking. @stubailo, what do you think? If having a clean context is desirable, we should probably expose a factory-like function for this. Or find another way to customize resetting the context between requests (maybe a |
|
Hmmmmm yeah this would be quite a large breaking change for sure. What would the factory function approach look like? I think we definitely need to have the ability to have the same context for all requests. |
|
Maybe we could allow For backwards compatibility, we could replace any provided non-function |
|
I am happy to allow context to be a function and have it default to Object.assign if it is not a function, that solves backwards compat |
|
@bennyhobart This seems like a reasonable solution, so it would be great if you wanted to work on implementing it! |
| if (isFunction(context)) { | ||
| context = context(); | ||
| } else if (isBatch) { | ||
| context = Object.assign({}, context || {}); |
There was a problem hiding this comment.
Would it make sense to make this a shallow clone that respects the prototype? Something like Object.assign(Object.create(Object.getPrototypeOf(context)), context)? That would solve the issues people are having with class-based contexts. But maybe that is too much magic and we should steer people towards using a function.
There was a problem hiding this comment.
If we want the default behaviour to be cloning context for batch requests, then your solution will work for more users out of the box!
If anything I'd consider remove cloning because it's not clear it occurs when reading the API (unless i missed something)
| @@ -27,6 +27,10 @@ function isQueryOperation(query: DocumentNode, operationName: string) { | |||
| return operationAST.operation === 'query'; | |||
| } | |||
|
|
|||
| export function isFunction(arg: any): arg is Function { | |||
There was a problem hiding this comment.
It seems this doesn't have to be exported.
bennyhobart
force-pushed
the
master
branch
3 times, most recently
from
3d531b6
to
b459b2f
Compare
| if (isFunction(context)) { | ||
| context = context(); | ||
| } else if (isBatch) { | ||
| context = Object.assign({}, Object.getPrototypeOf(context), context); |
There was a problem hiding this comment.
I think what you'd want here is Object.assign(Object.create(Object.getPrototypeOf(context)), context), because Object.assign({}, Object.getPrototypeOf(context), context) doesn't actually set the prototype, it just copies the properties from the prototype.
There was a problem hiding this comment.
and i thought i knew js, sorry about that
Opened this to solve #461, let me know if this change is appropriate and I'll clean up for merge
TODO: