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

Type definition improvements. #288

Open
YourWishes opened this issue Jun 22, 2019 · 5 comments
Open

Type definition improvements. #288

YourWishes opened this issue Jun 22, 2019 · 5 comments

Comments

@YourWishes
Copy link
Contributor

YourWishes commented Jun 22, 2019

I've been working with the API and I have a few ideas for type definition improvements, I'll submit a few pull requests when I can find some time as I've made a few of these already.

Fixes

  • ICarrierService - Needs id:number as part of the interface
  • ICreateArticle - image is only assignable to type of IBase64Image, when IShopifyImage could also be accepted.
  • ICheckoutDiscount - Missing optional code string.

Improvements to types

  • ICollection - Interface for a standard collection model, since ISmartCollection and ICustomCollection have a fair bit of overlap, also the various collection webhooks e.g. /collections/create return essentially either ISmartCollection | ICustomCollection
  • Better use of params for various functions, e.g. product.list currently accepts params?:any
  • Better use of similar object types. e.g. ILineItemProperty vs ICheckoutLineItemProperty, essentially doing the same thing.

Additions

  • WebhookRequestMap - Interface for lookup of WebhookTopic to an interface of webhook request. e.g. WebhookTopic products/create can have request of IProduct. This will require additions to what can be given by a webhook, e.g.
    • ICart - Interface for cart webhook callbacks
    • IDeleteItem - Interface for a webhook callback for deleted item, generally { id:number }
    • ICollection - Refer to improvements to types
  • ICarrierRequest - Interface for carrier request callbacks, will also require ICarrierItem and ICarrierAddress
  • ICarrierResponse - Interface for carrier responses to Shopify
  • RequestError (or similar) - Enumerator of errors that Shopify may return.
  • IOrderLineItem - Seems to be missing a bit of stuff, particularly discount_applications
  • AccessScopes - Enumerator of possible access scopes, also useful for accessScope.list
    • This could potentially lead to having some form of precheck for available access scopes and giving us type definitions depending on access scope availibility.

Improvements to structure

  • At the moment all the types are defined in the master index.d.ts file, it can be a bit much to navigate, perhaps a better defined folder structure would help with additions and changes.

I guess there's also a case to be made for converting the project from Javascript to Typescript, if this is something that is being considered.

@DaveLo
Copy link

DaveLo commented Nov 26, 2019

I was going to create an issue related to Pagination, but this seems like the right place to add.

My static type checker is unhappy with order.nextPageParameters as the response is defined as IOrder[]. I'm not super knowledgeable in typescript, but I think maybe something like:

interface IOrderList extends Array<IOrder> {
  nextPageParameters?: string,
  previousPageParameters?: string,
}

Alternatively maybe a response interface that could contain any of the typed array responses so as not to need to add a list type for every endpoint with a list api call?

@bags1976
Copy link

bags1976 commented Nov 29, 2019

I had a similar issue with paginated results on products. Something as simple as a generic this

interface IPaginatedResult<T> extends Array<T> {
		nextPageParameters?: any;
		previousPageParameters?: any,
}

Then change the return types for all paginated responses (not all are currently implemented according to the docs as of today).

order: {
    cancel: (id: number, params?: any) => Promise<Shopify.IOrder>;
    close: (id: number) => Promise<Shopify.IOrder>;
    count: (params?: any) => Promise<number>;
    create: (params: any) => Promise<Shopify.IOrder>;
    delete: (id: number) => Promise<any>;
    get: (id: number, params?: any) => Promise<Shopify.IOrder>;
    list: (params?: any) => Promise<IPaginatedResult<Shopify.IOrder>>;
    open: (id: number) => Promise<Shopify.IOrder>;
    update: (id: number, params: any) => Promise<Shopify.IOrder>;
  };

product: {
    count: (params?: any) => Promise<number>;
    create: (params: any) => Promise<Shopify.IProduct>;
    delete: (id: number) => Promise<void>;
    get: (id: number, params?: any) => Promise<Shopify.IProduct>;
    list: (params?: any) => Promise<IPaginatedResult<Shopify.IProduct>>;
    update: (id: number, params: any) => Promise<Shopify.IProduct>;
  };

In addition there is currently no support in the types for bigInt. It appears as though you are handling sending and retrieving the data with json-bigint but the types sending data are defined as number which will be a problem in the near future. A native bigint type was added in Typscript 3.2 but targets the native esnext bigint type and would not be compatible with the bigInt library used currently.

 fulfillment: {
    cancel: (orderId: bigint, id: number) => Promise<Shopify.IFulfillment>;
    complete: (orderId: bigint, id: number) => Promise<Shopify.IFulfillment>;
    count: (orderId: bigint, params?: any) => Promise<number>;
    create: (orderId: bigint, params: any) => Promise<Shopify.IFulfillment>;
    get: (
      orderId: bigint,
      id: number,
      params?: any
    ) => Promise<Shopify.IFulfillment>;
    list: (orderId: bigint, params?: any) => Promise<Shopify.IFulfillment[]>;
    open: (orderId: bigint, id: number) => Promise<Shopify.IFulfillment>;
    update: (
      orderId: bigint,
      id: number,
      params: any
    ) => Promise<Shopify.IFulfillment>;
  };

@YourWishes
Copy link
Contributor Author

I think it could be beneficial to have something similar to;

type IPaginatedResults<T> = T & {
  nextPageParameters?:T & IPaginatedResults<T>;
  previousPageParameters?:T & IPaginatedResults<T>;
}

//Then:
let response = order.list({ limit: 50 });
type X = typeof response;//Shopify.IOrder & IPaginatedResults<{ limit:number }>;

I assume the TS Compiler will struggle with this recursive type definition a bit, but something similar came to mind.

@lpinca
Copy link
Collaborator

lpinca commented Dec 1, 2019

I don't use TypeScript so any help is appreciated. Feel free to open PRs to improve the type definitions.

@lpinca lpinca mentioned this issue May 10, 2020
LarsBuur added a commit to LarsBuur/Shopify-api-node that referenced this issue Dec 19, 2020
No original work here. It´s basically what @YourWishes suggests in MONEI#288.
Could and should be extended throughout the api for lists.

Usage example:
```
  const orderList: IOrder[] = [];
  const orderStatus = 'any';

  let params = { status: orderStatus, limit: 250 };

  do {
    const orders = await shopify.order.list(params);

    for (const order of orders) {
      orderList.push(order);
    }

    params = orders.nextPageParameters;
  } while (params !== undefined);

  console.log(`after while loop len:${orderList.length}`)
```
@ozzyonfire
Copy link
Contributor

I also noticed quite a few type definitions are out of date.
Specifically;

  • IOrder is missing properties like admin_graphql_api_id (and others)
  • IOrderFulfillment is missing properties like name
  • IOrderFulfillmentLineItem is missing the discount_allocations property.

I'm happy to submit a pull request when I get a chance, but I was wondering if this would impact users on a different API version? and how we would go about making that distinction...

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

No branches or pull requests

5 participants