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

Make language nodes immutable #1338

Merged
merged 4 commits into from
Jul 9, 2018
Merged

Conversation

tenderlove
Copy link
Contributor

We have code in our client that "deep freezes" all AST nodes. I think we could eliminate that if we just made AST nodes immutable by default. I only found one place where the gem is mutating nodes, but I guess all those "writer" methods are public API, so this PR might only be good for a major version release.

I separated this PR to two commits because I think avoiding mutation internally has merit on its own.

Now we don't have to mutate the `InputValueDefinition` node after it's
allocated.
Make all AST nodes "read-only" data structures
@rmosolgo
Copy link
Owner

👍 Looks like a nice improvement to me, I want to check tomorrow to make sure it doesn't break graphql-client, but other than that, I think it's all clear!

@rmosolgo
Copy link
Owner

Somehow I thought I already merged this, but I didn't 😌

It's going to break graphql-client:

https://github.com/github/graphql-client/blob/master/lib/graphql/client/query_typename.rb#L21-L34

I like the idea of a more stable AST structure, but I'm going to change the target of this PR to 1.8-dev and merge it in a bit, when I have some time to update that code as well.

@tenderlove
Copy link
Contributor Author

I'll look in to what it takes to make graphql-client not mutate nodes. We may need a more functional interface for manipulating ASTs.

@rmosolgo
Copy link
Owner

GraphQL-js has a really nice approach for this, I added a comment to the previous issue for improving AST manipulation: #455 (comment)

@rmosolgo rmosolgo changed the base branch from master to 1.9-dev July 9, 2018 18:54
@rmosolgo rmosolgo added this to the 1.9.0 milestone Jul 9, 2018
@rmosolgo
Copy link
Owner

rmosolgo commented Jul 9, 2018

There are still a few things to do before immutable nodes are really ready:

  • consider freezing the arrays that make up.children, too, so it doesn't get changed underfoot (then .children could also be frozen)
  • add Node#path (and maybe Node#parent?) to simplify working with the AST, and it can be quite efficient now that they're frozen
  • improve Visitor (see [Language::Visitor] use method-based visit handlers #1290) to support manipulating the AST, so people have a new way of messing with hand-rolled ASTs

But this is a great first step, so merging into the dev branch!

@rmosolgo rmosolgo merged commit 255842c into rmosolgo:1.9-dev Jul 9, 2018
@rmosolgo rmosolgo mentioned this pull request Jul 9, 2018
7 tasks
rmosolgo pushed a commit that referenced this pull request Sep 13, 2018
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.

None yet

2 participants