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

admin-graphql-api-utilities: Add Gid type #2622

Merged
merged 8 commits into from May 18, 2023
Merged

admin-graphql-api-utilities: Add Gid type #2622

merged 8 commits into from May 18, 2023

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Mar 30, 2023

Description

This PR adds a Gid type that can be used by consumers of composeGid and composeGidFactory to provide strongly-typed GID values. This makes it a lot less likely for one GID type to accidentally be used in place of another.

For example, with the current API, this is perfectly fine:

async function getOrder(gid: string) {
 // ...
}

// Oops! This should say 'Order'!
const orderId = composeGid('Orde', 123);

// Runtime Error! 😭
await getOrder(orderId);

However, with this change we can easily make this catch the typo as a compile-time error:

async function getOrder(gid: Gid<'shopify', 'Order'>) {
 // ...
}

// Oops! This should say 'Order'!
const orderId = composeGid('Orde', 123);

// Compile-time Error! 🎉
await getOrder(orderId);

Additionally, since Gid can implicitly cast down to string, this change should only affect those who opt into this behavior.

Possible Additions

  1. We could also introduce a Shopify-specific Gid type to reduce the amount of typing1 needed for users using this with the Shopify API:

    type ShopifyGid<T extends string> = Gid<'shopify', T>; 
  2. We could also include type guards like isGidFactory and isGid to take in a string and check if it's the desired Gid type:

    Code Sample
    export function isGidFactory<N extends string>(namespace: N) {
      return function isGid<T extends string = unknown>(
        gid: string,
        key?: T,
      ): gid is Gid<N, T> {
        if (!gid.startsWith(`gid://${namespace}/`)) {
          return false;
        }
    
        try {
          if (key !== undefined && parseGidType(gid) !== key) {
            return false;
          }
    
          parseGid(gid);
        } catch {
          return false;
        }
    
        return true;
      };
    }

Any thoughts on these?

Footnotes

  1. By "reduce the amount of typing", I'm referring to reducing the amount of keystrokes and potential typos in non-autocomplete scenarios. From my experience, IDEs won't autocomplete strings like 'shopify', but they will autocomplete types like ShopifyGid (normally even with just a couple characters like SG).

@MrGVSV MrGVSV requested a review from a team as a code owner March 30, 2023 19:45
@MrGVSV MrGVSV requested a review from znja March 30, 2023 19:45
export type Gid<
Namespace extends string,
Type extends string,
> = `gid://${Namespace}/${Type}/${number | string}`;
Copy link

@gardenm gardenm Apr 26, 2023

Choose a reason for hiding this comment

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

I think this is great. It leverages Typescript really well to improve type-safety.

Copy link

@gardenm gardenm left a comment

Choose a reason for hiding this comment

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

I think the ShopifyGid idea makes sense since the exported composeGid is using 'shopify' as the namespace.

I don't have a strong feeling about the isGidFactory and isGid ideas.

@MrGVSV
Copy link
Member Author

MrGVSV commented May 1, 2023

I think the ShopifyGid idea makes sense since the exported composeGid is using 'shopify' as the namespace.

I don't have a strong feeling about the isGidFactory and isGid ideas.

I'll add these in a separate commit. If we decide we don't want them, that commit can just be reverted before merging.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Code looks great! Just some small housekeeping needed:

Can you pease add a changeset using yarn changeset to generate a changelog entry. As you're adding new features, this is a minor change.

As you're adding some type strictness, you might be interested in adding type tests (this will need a rebase atop #2652 to get it working with zero extra effort). Not critical, but might be a neat way of proving the strictness your adding.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

I've took the liberty of adjusting the changeset to be more descriptive. Thanks for the PR!

@BPScott BPScott merged commit e44bc03 into main May 18, 2023
10 checks passed
@BPScott BPScott deleted the mrgvsv/gid-type branch May 18, 2023 12:31
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

4 participants