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

quoteCombine not returning error as expected #150

Closed
ytsruh opened this issue Apr 27, 2021 · 6 comments
Closed

quoteCombine not returning error as expected #150

ytsruh opened this issue Apr 27, 2021 · 6 comments
Labels
bug Something isn't working

Comments

@ytsruh
Copy link

ytsruh commented Apr 27, 2021

Bug Report

Describe the bug

Running the quoteCombine method within a loop I am able to return data as expected until I reach a symbol that cannot be found. Instead of returning an Error that can be caught in try/catch. Nothing appears to be returned. Typically my list of symbols is kept upto date but I'm unable to guarantee this is 100% in line with what Yahoo has

Minimal Reproduction

Calling await getData causes my server to 'hang' and eventually time out

`const getData = async (array) => {

  let x = [];
  let y = [];
  for (const row of array) {
    try {
      console.log(row.symbol);
      const result = await yahooFinance.quoteCombine(`${row.symbol}.L`);
      console.log(result);
      x.push({ [row["symbol"]]: result });
    } catch (err) {
      console.log(err);
     y.push(err)
    }
  }
  return { x , y };
};`

Environment

Browser or Node: Node
Node version (if applicable): 14 & 15
Npm version: 7.4
Browser verion (if applicable): N/A
Library version (e.g. 1.10.1):

Additional Context

@ytsruh ytsruh added the bug Something isn't working label Apr 27, 2021
@gadicc
Copy link
Owner

gadicc commented Apr 28, 2021

Hey @ytsruh, thanks for reporting this.

Looks like Yahoo's API silently skips missing symbols.

I'll add a check at the end of the request to look for any symbols we asked for and didn't get back, and resolve() such cases with undefined, which is how quote() handles these symbols on a single call.

Might only get to this next week but watch this space :D

@gadicc
Copy link
Owner

gadicc commented Apr 28, 2021

This is fixed in devel but will probably only get released next week.

Also want to look at:

  • What happens if quoteCombine() is called again after timeout but before previous request returns.
  • Should add similar undefined test for regular quote()
  • Maybe have an option for quote() to return a map / object (will also allow for cleaner code in our fix for quoteCombine()

@ytsruh thanks again for reporting.

@ytsruh
Copy link
Author

ytsruh commented Apr 28, 2021

Honestly.....thank you. I was convinced it was something I was/wasn't doing.

Will keep an eye on this and test when released to report back. Appreciate the quick response as well! Happy to help if/however I can

gadicc pushed a commit that referenced this issue Jun 1, 2021
# [1.11.0](v1.10.4...v1.11.0) (2021-06-01)

### Bug Fixes

* **deps:** update dependency ajv to ^8.1.0 ([1769641](1769641))
* **quoteCombine:** resolve `undefined` for missing symbols ([#150](#150)) ([f8c25e3](f8c25e3))
* **testing:** specify jest.js path, not bin ([#170](#170)) ([6772c8e](6772c8e))

### Features

* **options:** accept `date` option ([#186](#186)) ([11b8a72](11b8a72))
* add (friendly) warning when used in the browser ([3c4c5a0](3c4c5a0)), closes [#153](#153)
* add insights module ([#169](#169)) ([4603232](4603232))
* **concurrency:** ability to limit max simultaneous requests ([#76](#76)) ([3424d44](3424d44))
* **modules:** build (true) esm, (interop) cjs modules; tests/readme ([#144](#144)) ([2182f6c](2182f6c))
* **setGlobalConfig:** add setGlobalConfig function ([#133](#133)) ([43ebaa4](43ebaa4))
@gadicc
Copy link
Owner

gadicc commented Jun 1, 2021

Hey @ytsruh, we just released all of last months "devel" changes in 1.11.0. Let us know how it goes. I'm still leaving this issue open as there are some linked issues (particularly #151), which hopefully I can finish up soon now that a number of other higher priorities have been released.

@ytsruh
Copy link
Author

ytsruh commented Jul 29, 2021

Sorry, this has taken me a long time to look at. However, pleased to say this seemed to do the trick!

I didnt have any issues when a symbol is 'not found'. So thanks for the fix!

@gadicc
Copy link
Owner

gadicc commented Jul 30, 2021

Hey @ytsruh, no worries at all, and thanks for reporting back!

May you be blessed with ever resolving symbols, always ;)

@gadicc gadicc closed this as completed Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants