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

feat: add option to avoid async path in type resolvers #897

Closed

Conversation

StarpTech
Copy link

@StarpTech StarpTech commented May 27, 2021

This will fix zalando-incubator/graphql-jit#90 and optimize a hot path. Async resolvers are rarely used.

@MichalLytek
Copy link
Owner

Closing as discussed in #516 🔒

Unfortunately, your solution is even worse because you've hardcoded the sync path by schema options.

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Out of scope ✖️ Proposal doesn't fit in project goals labels May 27, 2021
@StarpTech
Copy link
Author

StarpTech commented May 27, 2021

Unfortunately, your solution is even worse because you've hardcoded the sync path by schema options.

Code-duplication can be reduced. It's a workaround for an edge case.

According to #516 Why is it out-of-scope? Creating promises at this place is pure overhead.

Either way, forcing every resolveType resolver to return a promise is worse for performance. Why not let the developer choose whether they want it to return a promise or not?

Same here.

@MichalLytek
Copy link
Owner

As you can see, introducing sync paths results in duplicated code. TypeGraphQL was not designed with this kind of optimization in mind but the vNext is.

@StarpTech Please use your own fork for now.

@StarpTech
Copy link
Author

StarpTech commented May 27, 2021

@StarpTech Please use your own fork for now.

That's the worst case. Are you working on a solution? The best solution would be to pass the resolveType function as it is.

@MichalLytek
Copy link
Owner

MichalLytek commented May 27, 2021

The best solution would be to pass the resolveType function as it is.

How would you return a GraphQLObjectType instance if TypeGraphQL build schema from classes with decorators, not graphql-js constructs like GraphQLObjectType? See what getResolveTypeFunction method is doing.

@MichalLytek
Copy link
Owner

Ah right, it's now deprecated because of issues in transforming the schema, so now the object type name via string is recomended.

Still we need to support returning object type class, same way as we need to support returning promise in resolveType.

@StarpTech
Copy link
Author

StarpTech commented May 27, 2021

Still we need to support returning object type class, same way as we need to support returning promise in resolveType.

Something like this:

import { ResolveTypeSelector } from "type-graphql";

// Options 1

const SearchResultUnion = createUnionType({
  name: "SearchResult",
  types: () => [Movie, Actor] as const,
  // with object type class but selector functions must be sync
  // ResolveTypeSelector will return a function that is used internally to filter for object type classes
  resolveType: new ResolveTypeSelector([
		[(val) => "rating" in value, Actor],
		[(val) => "age" in value, Actor]
	])
});

// Options 2 "raw" mode

const SearchResultUnion = createUnionType({
  name: "SearchResult",
  types: () => [Movie, Actor] as const,
  // Only strings are allowed can be passed to GraphQLUnionType as it is. Supports promises out of the box.
  resolveType: async (value: any, info?: GraphQLResolveInfo) => {
    if ("rating" in value) {
      return "Movie";
    }
    if ("age" in value) {
      return "Actor";
    }
    return undefined;
  },
});

Does it makes sense?

@MichalLytek
Copy link
Owner

And how this can be provided without making breaking changes in existing users' codebases?

@StarpTech
Copy link
Author

And how this can be provided without making breaking changes in existing users' codebases?

Good point and no real answer to this question because I don't know your user's "customers" relationship. Honestly, breaking changes should be avoided but it's no reason to block improvements in the long term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Out of scope ✖️ Proposal doesn't fit in project goals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue when returning Union (Abstract type)
2 participants