Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added test to readQuery #3838

Merged
merged 8 commits into from
Oct 27, 2018
Merged

added test to readQuery #3838

merged 8 commits into from
Oct 27, 2018

Conversation

CarloPalinckx
Copy link
Contributor

@CarloPalinckx CarloPalinckx commented Aug 19, 2018

Adds a test to ensure that passed in arguments aren't mutated after a readQuery as requested per issue: #2464

Fixes #2464.

@apollo-cla
Copy link

@CarloPalinckx: 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/

@xpol
Copy link

xpol commented Aug 21, 2018

It would be better use 'deep-freeze' package to ensure the argument is not changeable.

@CarloPalinckx
Copy link
Contributor Author

CarloPalinckx commented Aug 21, 2018

It would be better use 'deep-freeze' package to ensure the argument is not changeable.

I think I don't really agree on this. Not mutating arguments that are passed by reference is a pretty common best practice. With this added test we at least confirm that it doesn't happen. Which should be enough imo.

@hwillson hwillson self-assigned this Oct 3, 2018
},
};

const preQueryCopy = { ...args };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean for this to be a deep copy? The { ...obj } syntax just copies the top level of the object.

Copy link
Contributor Author

@CarloPalinckx CarloPalinckx Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I did! good spot. I see lodash is already installed so i'll just use cloneDeep here.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CarloPalinckx!

@hwillson hwillson merged commit 2eff863 into apollographql:master Oct 27, 2018
@CarloPalinckx CarloPalinckx deleted the readquery-test branch October 29, 2018 16:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants