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

feat: allow withMutation() nesting #456

Merged
merged 1 commit into from
Feb 10, 2019

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Feb 10, 2019

Number 3 from #454

mixed().withMutation(schema => {
  schema.withMutation(changeA);
  changeB(schema);
}); // now changeB gets mutable instance

@jquense
Copy link
Owner

jquense commented Feb 10, 2019

What would anyone need to do this?

@vonagam
Copy link
Contributor Author

vonagam commented Feb 10, 2019

Right now withMutation() limited only to constructor, but if you want to use it anywhere else in codebase to reduce number of cloning (deep clones with lodash are not cheep) you need to be sure that it can be nested:

schema.withMutation(schema => {
  schema.methodA(); // if methodA uses withMutation()
  schema.test({...}); // then this will have no effect on schema
});

@vonagam
Copy link
Contributor Author

vonagam commented Feb 10, 2019

withMutation() is part of public api as it is documented in readme. And inability to use it inside itself is not mentioned. So this is kinda bug, which is fixed by one line.

@jquense
Copy link
Owner

jquense commented Feb 10, 2019

I agree it's technically a bug, still worth understanding the use case since i've got to maintain it. I don't think the above is very convincing as a use-case but sure.

@jquense jquense merged commit e53ea8c into jquense:master Feb 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants