Uh oh!
There was an error while loading. Please reload this page.
Add extern_enums and skip codegen for large enums - #56
Merged
Conversation
extern_enums, apply it to large enumsextern_enums and skip codegen for large enumsjbourassa
commented
Mar 7, 2024
Comment on lines
+88
to
+91
| - `query_path`: A path to a GraphQL query, whose result will be used | ||
| as the input for the function invocation. The query MUST be named "Input". | ||
| - `schema_path`: A path to Shopify's GraphQL schema definition. Use the CLI | ||
| to download a fresh copy. |
ContributorAuthor
There was a problem hiding this comment.
This is now the same copy as the macro's Rustdoc.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The `generate_types` macro didn't use `parse_macro_input!` to parse its arguments, unlike other macros. This unifies arg parsing across macros, making it easier to add a field to all macros.
davejcameron
approved these changes
Mar 8, 2024
dphm
approved these changes
Mar 8, 2024
dphm
left a comment
Contributor
There was a problem hiding this comment.
Great! Love the macro parsing refactor 😃
What's the behaviour for an enum that doesn't exist?
Uh oh!
There was an error while loading. Please reload this page.
The `extern_enums` arg is forwarded to the `graphql-client` crate, which will use it as the type for the enum instead of generating code for it. By default, the 3 large enums from Function APIs are passed as `extern_enums`: LanguageCode, CountryCode, CurrencyCode. Users can override that by sending `extern_enums` explicitly.
jbourassa
commented
Mar 8, 2024
ContributorAuthor
The behaviour of the underlying crate, which is:
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Codegen for large enums bloat the Wasm binary size on top of producing not-very-efficient deserializing code.
This PR adds an
extern_enumsarg to the relevant macros. Theextern_enumsarg is forwarded to thegraphql-clientcrate, which will skip codegen for those enums. The type is expected to already be defined, with e.g.type CountryCode = String;.By default, the 3 large enums from Function APIs are defined as Strings and passed as
extern_enums: LanguageCode, CountryCode, CurrencyCode. Users can override that by sendingextern_enumsexplicitly.Decisions:
extern_enums = ["Foo", "Bar"]instead of graphql-client'sextern_enums("Foo", "Bar"). I thought this was more aligned with the other attributes.DiscountApplicationStrategy), and only problematic for large enums.Note: this PR is best reviewed commit-by-commit.