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

Missing google_shopping engine from EngineMap #5

Closed
pawKer opened this issue Feb 1, 2023 · 6 comments
Closed

Missing google_shopping engine from EngineMap #5

pawKer opened this issue Feb 1, 2023 · 6 comments

Comments

@pawKer
Copy link

pawKer commented Feb 1, 2023

Hi,

I was trying to use the "google_shopping" engine in the getJson function but it looks like it is not in the EngineMap yet.

I would just like to know if this is going to be added soon or if I should use the previous version of the library?

@hartator
Copy link

hartator commented Feb 2, 2023

Maybe we can have enginebe just as another param instead of separating as a function argument. That matches how /search.json treats engines and we don't have the risk of npm package running behind the actual API.

For example, instead of:

import { getJson } from "serpapi";
const response = await getJson("google", {
  api_key: API_KEY, // Get your API_KEY from https://serpapi.com/manage-api-key
  q: "coffee",
  location: "Austin, Texas",
});
console.log(response);

have:

import { getJson } from "serpapi";
const response = await getJson({
  api_key: API_KEY, // Get your API_KEY from https://serpapi.com/manage-api-key
  engine: "google", 
  q: "coffee",
  location: "Austin, Texas",
});
console.log(response);

@sebastianquek
Copy link
Contributor

@pawKer I've updated the library with the Google Shopping API types.

However, note that any string can be passed in as the engine argument; there isn't any validation within the library. So using "google_shopping" will work correctly.

It's only if you're using TypeScript that a warning will appear.


@hartator I tried that approach previously. Trade-off is we won't be able to get the nice TypeScript autocomplete. engine needs to be a function argument for autcompletions to work:

image

@pawKer
Copy link
Author

pawKer commented Feb 2, 2023

Thank you very much @sebastianquek! 🥳

@hartator
Copy link

@hartator I tried that approach previously. Trade-off is we won't be able to get the nice TypeScript autocomplete. engine needs to be a function argument for autcompletions to work:

Having a engine: "something-arbitrary" and being just another param are big parts of our lib rewrite goals. Can we rewrite this lib this way?

Regarding autocompletions, let's drop them if that means this library would have a different way to handle params than other libs. Anyway, types are already creating confusion what's supported or not. Maybe if you really want to keep autocompletions, let's just have every params together from every engines and for params that shared between engines (i.e., q) having on paragraph for each engine ([google] Some explanation [Bing] Some other explanation [Yandex] A third explanation). But, I think I rather drop it altogether as we still have issues if some params are added to SerpApi but not to the JS lib.

@sebastianquek
Copy link
Contributor

sebastianquek commented Feb 10, 2023

@hartator Any arbitrary string can be passed in as the engine argument, i.e. getJson("something-arbitrary", {...}) to get results.

One enhancement is to have a polymorphic getJson that can accept both an engine string or params object as the first argument. So if users want type-safety, pass in the engine string as the first argument, otherwise pass in the params object that will essentially be of Record<string, unknown> type.

But, I think I rather drop it altogether as we still have issues if some params are added to SerpApi but not to the JS lib.

I think excluding types is a step backwards. It provides a lot of hints as to what is accepted and it removes a whole class of errors related to sending the wrong params. A library should help users as much as possible, otherwise there's very minimal value in it. We already have the rake task to easily generate the types, so it's a matter of figuring out how to automate that further.

@sebastianquek
Copy link
Contributor

@hartator I've managed to find a better way

  • getJson/getHtml accepts search params as the first argument rather than engine.
  • Does not show warnings for arbitrary engines/search params.
  • Autocomplete for search params (note that there's no autocomplete for engine, but I think it's an acceptable tradeoff).
  • Type safety can still be maintained by using EngineParameters<...>.

I've raised 2 PRs:

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

3 participants