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

Separate interfaces from implementations for exported classes #3616

Closed
wants to merge 1 commit into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 29, 2022

= allows for custom implementations of the GraphQL entity implementations, e.g. the execute function from GraphQL can be used for any object that implements the GraphQLSchema interface, even if does not conform to the default GraphQLSchemaImpl class.
= currently provides symbols for predicates for the following interfaces: GraphQLScalarType, GraphQLObjectType,
GraphQLInterfaceType, GraphQLUnionType, GraphQLEnumType, GraphQLInputObjectType, GraphQLList, GraphQLNonNull, GraphQLDirective, GraphQLSchema, and Source, i.e. all classes that made internal use of the instanceOf method.
= the instanceOf method now uses symbols instead of instanceof.
= the exported BREAK object is now passed into the visitor function to allow for cross-compatibility between graphql library instances.

@netlify
Copy link

netlify bot commented May 29, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit e60c6f3
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/629914f929cc800008e88390
😎 Deploy Preview https://deploy-preview-3616--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR yaacovCR force-pushed the impl branch 2 times, most recently from eb98ae0 to 18e979f Compare May 29, 2022 11:38
@yaacovCR
Copy link
Contributor Author

This PR is meant to provide a direction and proof of for removing the reliance on instanceof within this library. The changed within likely probably could be broken up into several different smaller PRs.

For example, moving BREAK into the visitor method argument certainly could be its own change.

@yaacovCR yaacovCR mentioned this pull request May 29, 2022
@yaacovCR
Copy link
Contributor Author

tests not running, see #3614 (comment)

@yaacovCR
Copy link
Contributor Author

Note that we still warn (in development) if the objects do not conform to the GraphQL version within the peer dependency, by branding the version into the above object instances. The check is performed only at development, but the branding, for simplicity, happens in production as well.

It would be good to run benchmarks. It might pay to research whether we can skip the branding in production, which might require a build step to remove the branding.

@yaacovCR
Copy link
Contributor Author

Note that the current PR changes the class names to GraphQL*TypeImpl and has the class-free TS interfaces use just GraphQL*Type.

To lighten the breaking change set, we could keep the class names as GraphQL*Type and have the class-free TS interfaces use IGraphQL*Type.

@yaacovCR yaacovCR force-pushed the impl branch 2 times, most recently from 0e6b059 to bb17f06 Compare June 2, 2022 19:49
= allows for cross-realm/worker GraphQL
= allows for custom implementations of the GraphQL entity
implementations, e.g. the `execute` function from GraphQL can be used
for any object that implements the `GraphQLSchema` interface, even if
does not conform to the default `GraphQLSchemaImpl` class.
= currently provides symbols for predicates for the following
interfaces: `GraphQLScalarType`, `GraphQLObjectType`,
`GraphQLInterfaceType`, `GraphQLUnionType`, `GraphQLEnumType`,
`GraphQLInputObjectType`, `GraphQLList`, `GraphQLNonNull`,
`GraphQLDirective`, `GraphQLSchema`, and `Source`, i.e. all classes that
made internal use of the `instanceOf` method.
= the `instanceOf` method now uses symbols instead of `instanceof`.
= the exported BREAK object is now passed into the visitor function to
allow for cross-compatibility between `graphql` library instances.
@yaacovCR
Copy link
Contributor Author

Note that I removed the comment about how this can enable cross-realm graphql.

That is because I was referring to constructing a schema in one thread, executing in another. It turns out that (with more research I could have discovered that) it is impossible to pass functions to a worker, so although a worker thread can receive a request and return a result, there is no way to pass GraphQL*Type objects across threads. See #2801 (comment) -- thanks @IvanGoncharov for pointing this out.

This PR could be used to enable esm/cjs compatibility (avoiding/embracing the dual package hazard), but that can be saved in a more lightweight way by #3617 or some other solution.

So the only remaining real reason to enable this PR is to allow drop in replacements for schema creation, validation, execution, etc.

So, I am closing this PR for now, because the value of this has significantly decreased in my mind, and if I am able to get in some of the remaining refactors from graphql-executor, I have less of a need for it myself. We always have it as a reference if need be.

@yaacovCR yaacovCR closed this Jun 14, 2022
@IvanGoncharov
Copy link
Member

Just a note, in TS you can "implement class":

class A {
  a: string | undefined;
}
class B implements A {
  a: string | undefined;
}

const a: A = new B();

The downside, you need to implement all private properties.
But schema objects don't have that many private properties.
That said maybe one day we will switch to ECMAScript private properties when TS will learn to ignore them in the "implements" check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants