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

Support Multiple Query Include Values #84

Closed
nowlena opened this issue Oct 25, 2023 · 4 comments
Closed

Support Multiple Query Include Values #84

nowlena opened this issue Oct 25, 2023 · 4 comments
Labels
pending fix this issue is expected to be resolved by a dependency outside of this library

Comments

@nowlena
Copy link

nowlena commented Oct 25, 2023

Very much liking the package! I think we'll move to using it given it's complete generated type safety approach that should always align with the actual BigCommerce APIs. Well done!

As for this issue... It'd be great to allow multiple "include" values to be used:

  const product = await bc.v3.GET('/catalog/products/{product_id}', {
    params: {
      path: { product_id: 1 },
      header: { Accept: 'application/json' },
      query: { include: 'variants,images,primary_image' }, // this currently throws an error
    },
  });

If you're able to target the query "include" values and make them their own type I believe you could do something like below:

// generate stand alone types
type GetProductByIdQueryIncludeValue = "variants" | "images" | "custom_fields" | "bulk_pricing_rules" | "primary_image" | "modifiers" | "options" | "videos";
type GetProductByIdQueryInclude = `${GetProductByIdQueryIncludeValue},${GetProductByIdQueryIncludeValue}` | GetProductByIdQueryIncludeValue;

// rest operations
export interface operations {
  getProductById: {
    parameters: {
      query?: {
        include?: GetProductByIdQueryInclude; // use our generated stand-alone type
      };
    };
  };
};

I'm sure there's a generic that can be written to improve things but I think you get the point.

Thanks!

@matthewvolk
Copy link
Owner

Thanks so much for the feedback, Aaron! Very happy to hear that the client's been of use to you 🙂

Apologies for the delay in getting back to you, just got back from a couple of weeks on travel for work. I like your suggestion a lot, and will play around with some generics/the generate script to see if I can automate this! Hope to get back to you soon 👍

@matthewvolk
Copy link
Owner

Hi @nowlena!

I opened a pull request in the bigcommerce/api-specs repository that should fix the issue you raised:

bigcommerce/api-specs#1527

If the PR is accepted, the include query parameter will be updated to accept a value of string[], such as:

const product = await bc.v3.GET("/catalog/products/{product_id}", {
  params: {
    path: { product_id: 77 },
    header: { Accept: "application/json" },
    query: { include: ["options", "images", "primary_image", "variants"] },
  },
});

Before (error):

br_84_before

After (no error):

br-84-after

I wanted to first try to fix the root cause of this issue at its source, hoping that the fix may help other developers using the api-specs repo for their own use cases; but, in the event that my PR can't be merged due to some unintended side effect, I'll re-explore a workaround implemented directly in this library like the one you suggested 🙂

@matthewvolk matthewvolk added the pending fix this issue is expected to be resolved by a dependency outside of this library label Nov 21, 2023
@matthewvolk
Copy link
Owner

Hi again, @nowlena! 👋

After a good bit of hard work from the BigCommerce DevDocs team, the team has gone through and fixed this problem for all query parameters in bigcommerce/docs#221.

To incorporate this work, I just released [email protected] to NPM, and after updating locally, I'm able to send multiple query parameters as a typesafe array:

bc.v3
  .GET("/catalog/products/{product_id}", {
    params: {
      path: { product_id: 77 },
      header: { Accept: "application/json" },
      query: { include: ["variants", "images", "primary_image"] },
    },
  })
  .then((res) => {
    console.log(res.data);
  });

If you have a chance, feel free to test this on your end and let me know if you run into any problems! I'll mark this issue as completed in a couple weeks if I don't hear back.

All the best 🍻

@nowlena
Copy link
Author

nowlena commented Apr 22, 2024

Works perfect!

Thank you and the other BC members who helped out, what a great DX!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending fix this issue is expected to be resolved by a dependency outside of this library
Projects
None yet
Development

No branches or pull requests

2 participants