Don't mutate options object when calculating variables from props#1968
Don't mutate options object when calculating variables from props#1968hwillson merged 5 commits intoapollographql:masterfrom
Conversation
hwillson
left a comment
hwillson
left a comment
There was a problem hiding this comment.
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!
| 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.
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.
ksmth
left a comment
ksmth
left a comment
There was a problem hiding this comment.
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) ...
ghost
mentioned this pull request
2 participants
This small change fixes #1873