-
Notifications
You must be signed in to change notification settings - Fork 787
Don't mutate options object when calculating variables from props #1968
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @ksmth! I'm going to adjust this a bit (see my comments inline), but we're close to getting this merged. It's also quite clear that we're lacking tests here, so I'll get something in place. Thanks again!
@@ -51,7 +51,7 @@ export function query< | |||
render() { | |||
let props = this.props; | |||
const shouldSkip = mapPropsToSkip(props); | |||
const opts = shouldSkip ? Object.create(null) : mapPropsToOptions(props); | |||
const opts = shouldSkip ? Object.create(null) : { ...mapPropsToOptions(props) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
can be passed into the graphql
hoc as either an object or a function. Spreading the result of mapPropsToOptions
here can happen in both cases - when options
is an object or a function. In the case of a function though, the return value of the options
function is already going to be a new object (see https://www.apollographql.com/docs/react/api/react-apollo.html#graphql-query-config-options). So we really only need to spread the result when we're using an options
object. I'll adjust this a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can easily pass a function that returns the object every time - thinking of a memoized function as an example. I don’t think it’s safe to assume that just because it’s a function it’ll always be a new object.
@ksmth Right, of course - it's just that in this case people are using the |
We should be good to go here @ksmth (left the spread as is). LGTM - thanks! |
* master: (112 commits) chore(deps): update dependency danger to v3.8.8 chore(deps): update dependency enzyme to v3.5.0 chore(deps): update dependency apollo-client to v2.4.1 chore(deps): update dependency apollo-cache-inmemory to v1.2.9 chore(deps): update dependency apollo-cache to v1.1.16 chore(deps): update dependency @types/react to v16.4.12 chore(deps): update dependency rollup-plugin-commonjs to v9.1.6 chore(deps): update dependency @types/node to v10.9.2 chore(deps): update dependency react-scripts to v1.1.5 chore(deps): update dependency ts-jest to v23.1.4 Avoid importing lodash directly (apollographql#2045) type graphql.options.skip HOC property (apollographql#2208) Replace duplicate ObservableQueryFields types defined in apollo-client (apollographql#2281) Make mock links mock parameter readonly (apollographql#2284) test-utils: allow passing a custom cache object to `MockedProvider` (apollographql#2254) Query: Fix data is undefined on error (apollographql#1983) Don't mutate options object when calculating variables from props (apollographql#1968) Feature: add onSubscriptionData callback to <Subscription> (apollographql#1966) Changelog update Example of a mutation including tests (apollographql#1998) ...
This small change fixes #1873