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

Typed base schema meta #17

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Typed base schema meta #17

wants to merge 1 commit into from

Conversation

emmatown
Copy link
Member

Unsure if I want to ship this.

Big open question around the all method:

  • Should it exist?
  • Should it have the "ignore the index signature from the Types type" behaviour so that it only accepts known type names?
  • Should it be called all? Other ideas:
    • strict
    • any
    • named

@emmatown emmatown requested a review from Copilot February 27, 2025 03:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces stronger TypeScript typings for the extension functions by parameterizing the schema metadata with a generic map of named types. It also adds a new "all" method to BaseSchemaMeta and updates the type signatures of schema wrapping helper functions.

Reviewed Changes

File Description
packages/extend/src/index.ts Added generic type parameter for schema metadata, updated helper functions to use stricter typings, and introduced the "all" method for accessing schema types

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

@@ -119,76 +119,99 @@ export function extend(
typeof extension === "function"
? extension({
schema,
all(name) {
Copy link
Preview

Copilot AI Feb 27, 2025

Choose a reason for hiding this comment

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

The 'all' method lacks an else branch to handle the case where 'getType(name as string)' does not match any of the known GraphQL type checks. Consider adding an error throw for unsupported types.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
object(name) {
const graphQLType = getType(name);
const graphQLType = getType(name as string);
Copy link
Preview

Copilot AI Feb 27, 2025

Choose a reason for hiding this comment

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

[nitpick] The repeated explicit cast 'name as string' might indicate that the 'getType' function's type signature could be refined to accept a more specific type, thereby reducing the need for such casts.

Suggested change
const graphQLType = getType(name as string);
const graphQLType = getType(name);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@emmatown emmatown force-pushed the typed-base-schema-meta branch from 6d919d5 to f24d07d Compare March 10, 2025 07:07
@emmatown emmatown changed the base branch from main to use-real-graphql-js-types March 10, 2025 07:07
@emmatown emmatown changed the base branch from use-real-graphql-js-types to easier-context-binding March 10, 2025 07:08
@emmatown emmatown force-pushed the typed-base-schema-meta branch from f24d07d to 5f0c5e7 Compare March 10, 2025 23:40
@emmatown emmatown force-pushed the easier-context-binding branch 3 times, most recently from 8c90521 to 646198e Compare March 11, 2025 06:00
@emmatown emmatown force-pushed the typed-base-schema-meta branch from 5f0c5e7 to a174199 Compare March 11, 2025 06:00
@emmatown emmatown force-pushed the easier-context-binding branch from 646198e to beb4055 Compare March 11, 2025 06:05
@emmatown emmatown force-pushed the typed-base-schema-meta branch 2 times, most recently from e83658f to b566ece Compare March 11, 2025 06:06
@emmatown emmatown force-pushed the easier-context-binding branch from beb4055 to af71214 Compare March 11, 2025 06:06
@emmatown emmatown force-pushed the typed-base-schema-meta branch from b566ece to 56375be Compare March 11, 2025 06:28
@emmatown emmatown force-pushed the easier-context-binding branch 2 times, most recently from 4acd1ff to bc4b76a Compare March 11, 2025 06:38
@emmatown emmatown force-pushed the typed-base-schema-meta branch from 56375be to 83f17ec Compare March 11, 2025 06:38
@emmatown emmatown force-pushed the easier-context-binding branch from bc4b76a to a2c76c6 Compare March 11, 2025 23:07
@emmatown emmatown force-pushed the typed-base-schema-meta branch from 83f17ec to 1fcf347 Compare March 11, 2025 23:07
@emmatown emmatown force-pushed the easier-context-binding branch from a2c76c6 to 75dcde4 Compare March 11, 2025 23:10
@emmatown emmatown force-pushed the typed-base-schema-meta branch from 1fcf347 to 46d3bb5 Compare March 11, 2025 23:10
@emmatown emmatown force-pushed the easier-context-binding branch from 75dcde4 to b2c3899 Compare March 11, 2025 23:23
@emmatown emmatown force-pushed the typed-base-schema-meta branch 2 times, most recently from 4d52d81 to d5e6cb9 Compare March 11, 2025 23:33
@emmatown emmatown force-pushed the easier-context-binding branch 2 times, most recently from 858fd76 to 6c9b7bf Compare March 11, 2025 23:36
@emmatown emmatown force-pushed the typed-base-schema-meta branch 2 times, most recently from 9e507e8 to 58b9bac Compare March 11, 2025 23:41
@emmatown emmatown force-pushed the easier-context-binding branch from 6c9b7bf to c7fac30 Compare March 11, 2025 23:41
@emmatown emmatown force-pushed the typed-base-schema-meta branch 2 times, most recently from 84cf8f4 to 4e204ba Compare March 12, 2025 03:05
@emmatown emmatown changed the base branch from easier-context-binding to main March 12, 2025 05:37
@emmatown emmatown force-pushed the typed-base-schema-meta branch from 4e204ba to 51e9c60 Compare March 13, 2025 23:09
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.

1 participant