Simplify directive mutation#967
Simplify directive mutation#967goto-bus-stop merged 4 commits intoapollographql:mainfrom tninesling:mutability-support
Conversation
crates/apollo-compiler/src/node.rs
Outdated
| impl<T: Clone> std::ops::DerefMut for Node<T> { | ||
| fn deref_mut(&mut self) -> &mut Self::Target { | ||
| self.make_mut() | ||
| } | ||
| } |
There was a problem hiding this comment.
The rest of this PR looks great but this change feels a bit "too magical" since it can make it invisible when a subtree stops being shared. Maybe that’s not so bad: our nodes don’t use interior mutability so sharing v.s. not wouldn’t change behavior and is "only" a performance concern. But it’s a departure from usual Rust conventions.
@goto-bus-stop what do you think?
There was a problem hiding this comment.
That's fair. I think this probably does obfuscate the copy-on-write behavior a bit too much. This is trying to resolve some ergonomic issues we're having when we try to mutate parts of the schema, but it probably isn't the right solution. When we get a handle to some field, and we want to mutate something inside, it becomes .make_mut() all the way down.
We're specifically trying to mutate schemas in-place, so I am slightly worried about the possibility that we run into a case where we try to make some update and an unexpected clone causes that not to stick (admittedly, hiding the make_mut in this way just makes that worse). Is there a way for us to get a non-Arc-ed schema expressly for in-place mutation and then freeze it afterwards?
3 participants
Implements mutable versions of some functions used to access directives and their arguments. This improves the ergonomics for some of the composition work we're doing in apollographql/router#7235.