Skip to content

[draft] graphql-parser: 0.2 -> 0.3 #395

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

Closed

Conversation

samuela
Copy link

@samuela samuela commented Oct 28, 2021

An attempt at upgrading the graphql-parser dependency to the latest version.

Unfortunately, this is a huge PITA due to fundamental changes in how graphql-parser handles the underlying text data. IIUC in previous versions Strings were just copied around and ownership of an AST node implied ownership of all the data necessary to print out what it is. However in the latest version, the seems to have changed to AST nodes only owning pointers with non-local lifetimes into the underlying text data. See eg https://docs.rs/graphql-parser/0.3.0/graphql_parser/query/trait.Text.html which is now the underlying trait for text data.

I'm not at all familiar with the graphql-client or graphql-parser codebases, so I only attempted a relatively naive "play-with-lifetimes" approach. I've sunk enough hours into this already, and I'm stuck at this point so putting this out there as a draft PR for anyone to extend/adapt/fix as they'd like!

@mathstuf
Copy link
Contributor

So I took a stab at this. It's really involved :( . I asked upstream if there was some way to get back the lifetime-less interface here: graphql-rust/graphql-parser#41

@mathstuf
Copy link
Contributor

Well, things are a lot better if you just ignore the lifetime support in the parser and use <'static, String> everywhere. I'll have a PR up shortly.

@mathstuf
Copy link
Contributor

See #396.

@mathstuf
Copy link
Contributor

Closing in preference for #396.

@mathstuf mathstuf closed this Nov 27, 2021
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