fix(api-graphql): generate client.models has no properties #12935
Conversation
| if (!config.API?.GraphQL) { | ||
| client.models = emptyModels as ModelTypes<never>; | ||
| generateModelsPropertyOnAmplifyConfigure<T>(client); | ||
| } else { | ||
| client.models = generateModelsProperty<T>( | ||
| client, | ||
| config.API?.GraphQL, | ||
| ); | ||
| } |
There was a problem hiding this comment.
In the case where we register the listener, it looks like it subscribes to config changes perpetually. I'm curious if this breaks expectations if a customer intends to create a client and then re-configure another one with another API.
On the other hand, if we don't intend to support that pattern, it makes sense to subscribe to config updates either way, IMO:
- Generate models, defaulting to empty object if not configured.
- Start listening for config changes.
An unlikely edge, I imagine. But I'm eager for clear-cut definition of the intended behavior that reconciles the differences.
HuiSF
force-pushed
the
hui/fix/api-graphql/empty-client-models
branch
from
65568ef to
3feedac
Compare
HuiSF
force-pushed
the
hui/fix/api-graphql/empty-client-models
branch
from
2589636 to
17e7554
Compare
HuiSF
force-pushed
the
hui/fix/api-graphql/empty-client-models
branch
from
17e7554 to
a3f531d
Compare
| expect(() => { | ||
| client.models.Todo.create({ name: 'todo' }); | ||
| }).toThrow( | ||
| 'Could not generate client. This is likely due to Amplify.configure() not being called prior to generateClient().', |
There was a problem hiding this comment.
Nit: could we expand on this error message? It may be a bit confusing since in this example configure was actually called, it's just missing a GraphQL configuration.
Something like: "... This is likely due to Amplify.configure() not being called prior to generateClient() or because the configuration passed to Amplify.configure() is missing GraphQL values"
| get() { | ||
| throw new Error( | ||
| 'Could not generate client. This is likely due to Amplify.configure() not being called prior to generateClient().', | ||
| ); | ||
| }, |
There was a problem hiding this comment.
script bundlers may randomly arrange their orders in the production bundle
Is it possible to have a race condition here, causing the error to get thrown because a customer accesses client.models.* before the configure hub event fired?
For example, I call Amplify.configure() with a valid config in one file and then in another:
const client = generateClient<Schema>();
const data = await client.models.Post.list();If the bundler arranges the files in such a way that the above snippet gets called first, will the customer see an error thrown?
There was a problem hiding this comment.
The race condition is going to happen when someone makes back-to-back calls exactly like this at the module level. For calls made at the module level, we would need to do the following in the same file to maintain the order of calls:
Amplify.configure();
const client = generateClient<Schema>();
const data = await client.models.Post.list();Let's discuss.
There was a problem hiding this comment.
Sounds good. Let's summarize in the PR description:
- Which usage patterns does this fix resolve?
- What are the remaining sharp edges?
iartemiev
dismissed
their stale review
Dismissing approval pending discussion
Description of changes
Amplify.configure()andgenerateClient()calls order can be randomly bundled by script bundler when they are located in separate files, as from the code path perspective they don't have a dependent relationship, however,generateClient()is actually depending onAmplify.configure()to get a valid GraphQL provider configuration. To resolve this, this PR added functionality togenerateClientas the following:generateClientgets called, if it can get a valid GraphQL provider configuration, it generates the properties forclient.models.coreHub event:configure, when the listener receives a valid GraphQL provider configuration, it generates the properties forclient.models. The listener modifiesclient.modelsby reference.Issue #, if available
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.