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

Generate Operation Variables & Field Arguments #2163

Merged
merged 25 commits into from
Feb 22, 2022

Conversation

AnthonyMDev
Copy link
Contributor

@AnthonyMDev AnthonyMDev commented Feb 15, 2022

No description provided.

@AnthonyMDev AnthonyMDev force-pushed the 1.0/operation-variables branch from d3dd12e to f9dcc92 Compare February 15, 2022 20:42
@AnthonyMDev AnthonyMDev marked this pull request as draft February 15, 2022 22:07
Copy link
Member

@calvincestari calvincestari left a comment

Choose a reason for hiding this comment

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

Glad we talked about the default value stuff, feels like we got it right in that conversation.

@AnthonyMDev AnthonyMDev marked this pull request as ready for review February 17, 2022 22:24
@AnthonyMDev AnthonyMDev changed the title Generate Operation Variables Generate Operation Variables & Field Arguments Feb 21, 2022
@AnthonyMDev
Copy link
Contributor Author

@trevorblades Not sure why Netlify deploy preview is failing. Can you look at this? Netlify logs are showing

Error: ENOSPC: System limit for number of file watchers reached, watch '/opt/b  uild/repo/docs/gatsby-config.js'

@trevorblades
Copy link
Contributor

@trevorblades Not sure why Netlify deploy preview is failing. Can you look at this? Netlify logs are showing

Error: ENOSPC: System limit for number of file watchers reached, watch '/opt/b  uild/repo/docs/gatsby-config.js'

Not sure why this happened, but I just re-ran the deploy and it seems to be working properly now. In the future, this kind of thing will be avoided since we'll only build docs deploy previews for PRs that change docs content.

@calvincestari calvincestari self-requested a review February 22, 2022 18:41
Copy link
Member

@calvincestari calvincestari left a comment

Choose a reason for hiding this comment

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

Nice work here, going to be great to get this out in the next alpha!

@@ -34,6 +34,10 @@ public enum GraphQLEnum<T: EnumType>: CaseIterable, Equatable, RawRepresentable
self = .case(caseValue)
}

public init(_ rawValue: String) {
Copy link
Member

@calvincestari calvincestari Feb 22, 2022

Choose a reason for hiding this comment

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

Should we just make this the designated initializer signature instead of having two public methods that do the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be misunderstanding you, but RawRepresentable requires you to have a public init(rawValue: String) initializer, so we can't do that.

@@ -75,7 +75,7 @@ public class GraphQLInputField: JavaScriptObject {

private(set) lazy var description: String? = self["description"]

lazy var defaultValue: Any? = self["defaultValue"]
lazy var defaultValue: GraphQLValue? = (self["astNode"] as JavaScriptObject)["defaultValue"]
Copy link
Member

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha thank you. This was painful to figure out... The other solution was going to require a lot of additional code in the TypeScript library that was going to further slow down the compilation of the AST. This is a little hacky, but definitely a better solution.

Long term, I'd really like us to re-write the JavascriptFrontend in Swift and just access the cold hard JavaScript objects in Swift directly from the start. It will be ugly and unsafe code, casting the JS objects directly without the TypeScript compiler checking and converting types for us, but it should improve performance further. Not urgent at all, but possibly a cool intern project or something haha. :)

@AnthonyMDev AnthonyMDev merged commit ad37c60 into release/1.0-alpha-incubating Feb 22, 2022
@AnthonyMDev AnthonyMDev deleted the 1.0/operation-variables branch February 22, 2022 20:05
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.

3 participants