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

add basic t.effectConnection implementation #11

Merged
merged 1 commit into from
Aug 6, 2023
Merged

Conversation

hayes
Copy link

@hayes hayes commented Aug 2, 2023

Got some questions about how to integrate the effect and relay plugins. Getting connections working with more complex field builders is a bit more involved, and requires the other plugin to specifically build out a connection method (you can work around it, but its a lot easier if the plugin supports it directly).

The strategies for interoperability are not really documented, and not at all obvious, so I wanted to open this PR as a starting place for supporting connections in the effect plugin

related to #3

Comment on lines +155 to +154
const connectionRef = connectionOptionsOrRef instanceof ObjectRef
? connectionOptionsOrRef
: this.builder.objectRef<ConnectionShape<SchemaTypes, unknown, boolean>>(
'Unnamed connection',
);
Copy link
Owner

Choose a reason for hiding this comment

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

What does Unnamed connection meaning and doing if connectionObjectOrRef is not instance of ObjectRef?

Here is my poor reasoning: Unnamed connection is just reference because .implement(...) is not called. If then, what happens in this.builder.configStore.associateRefWithName(connectionRef, connectionName)?

Copy link
Author

@hayes hayes Aug 3, 2023

Choose a reason for hiding this comment

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

basically, we need the ref before we know the name of the connection type. Refs always need an associated name, but we don't have a good name for the type at this point. If the field is never used, eg:

you do something like:

t.queryFields(t => {
  // this is not used
  t.connection({...})
  return {}
})

We never get the onFieldUse callback, and never update the ref with this.builder.configStore.associateRefWithName, and you may see an error like ref 'Unnamed connection' was not implemented when you build the schema

} as never);

if (!(connectionOptionsOrRef instanceof ObjectRef)) {
this.builder.configStore.onFieldUse(fieldRef, (fieldConfig) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Just for someone who didn't know about configStore.onFieldUse like me.

builder.configStore.onFieldUse Takes a field ref (returned by a field builder) and a callback to invoke once the config for the field is available.

https://pothos-graphql.dev/docs/guide/writing-plugins#useful-methods

},
);

this.builder.configStore.associateRefWithName(connectionRef, connectionName);
Copy link
Owner

Choose a reason for hiding this comment

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

Gotcha. so here is, "unnamed" connectionRef will be resolved here.

Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain for me about associateRefWithName? it looks not docummented api and I'm just reasoning about it from method name.

Copy link
Owner

Choose a reason for hiding this comment

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

(And let me know my guess is right or wrong 😉 )

Copy link
Author

Choose a reason for hiding this comment

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

yes, this is basically saying that the connectionRef should resolve to the type associated with connectionName. Internals get a little complicated due to backwards compatibility and historic reasons, but the implementations for types are stored in a map of name -> implementation, refs are stored as ref -> name, so to resolve a ref, the lookup is something like ref -> name -> implementation. This is a little weird because you need to delay the creation of the object type until after you have the name of the field to generate a type name that matches the field name. Normally you wouldn't need to use this method because you can just pass the ref into a method like builder.objectType

: '@pothos/plugin-relay is required to use this method';
}

export interface ConnectionShapeHelper<Types extends SchemaTypes, T, Nullable> {}
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose for adding these interfaces? I just tried remove these and everything is ok.

Copy link
Author

Choose a reason for hiding this comment

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

These are defined in the global namespace. This allows this plugin to reference the types without fully re-implementing them. for example ConnectionFieldOptions is empty here, but when the relay plugin is loaded, it will be extended with things like maxSize and other connection specific options

Copy link
Author

Choose a reason for hiding this comment

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

ConnectionShapeHelper is especially weird, it basically will be implemented as something like:

`interface ConnectionShapeHelper<Types extends SchemaTypes, T, Nullable>  {
  shape: ConnectionShape<Types, T, Nullable>

this in combination with this type

export type ShapeFromConnection<T> = T extends { shape: unknown } ? T['shape'] : never;

allows this plugin to get the shape of a connection

roughly { pageInfo: PageInfo, edges: { cursor: string; node: T }[] } (actual types are a bit more complicated to handle nullability/promises etc)

Copy link
Author

Choose a reason for hiding this comment

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

the overall goal is to limit what plugins know about the details of Connections while still being fully compatible

Copy link
Owner

@iamchanii iamchanii left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution and explains! please rebase your branch then will be merge.

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

3 participants