search

gadicc / node-yahoo-finance2

Unofficial API for Yahoo Finance
https://www.npmjs.com/package/yahoo-finance2
MIT License
393 stars 62 forks source link

Type issues since >= 2.12.0 #797

Open gadicc opened 1 month ago

gadicc commented 1 month ago

In 2.12.0, our new validation code landed. Although all tests were passing for functionality, we don't have any tests for the structure of typescript exports. This could result in compile errors or mismatched types for typescript users. We apologise for that. We believe most of these issues have either been fixed or are in the process of being fixed, and we'll leave this issue up for discoverability.

We know this is a pain and inconvenience, but was part of a big internal restructuring which greatly eases development and opens the way to non-node runtimes. So, we really thank the early adopters among you who took the time to provide feedback and help us iron out the final issues.

gadicc commented 1 month ago

As reported by @mtorregrosa in https://github.com/gadicc/node-yahoo-finance2/issues/795#issuecomment-2356056753:

For me, seems all fine in version 2.12.2, except that the following types have dissapeared:

import { HistoricalRowDividend, HistoricalRowHistory } from 'yahoo-finance2/dist/esm/src/modules/historical'

and

Furthermore, types seem to have changed from previous versions. A lot of numeric fields that previously where of type number, now are of type number | { raw: number }. For example: regularMarketPrice, beta, dividendYield, bookValue, etc.

cc: @Verdant31

gadicc commented 1 month ago

@mtorregrosa, thanks for your help and patience. Can you see if 2.12.13 fixes these final issues for you?

mtorregrosa commented 1 month ago

Thanks for the fixes in version 2.12.3:

New compilation problem found in version 2.12.3:

gadicc commented 1 month ago

Thanks for all your time with this, @mtorregrosa! I do understand it's a pain but the whole community is benefiting from your help :sweat_smile:

Umm, regarding your latest report... I think a few things might be going on here:

  1. The fields you mention were never actually "direct" fields in QuoteSummaryResult, you can see that in the original interface too (from before the recent changes). That you mentioned, they belong in:

    1. fullTimeEmployees is a property of the SummaryProfile and AssetProfile submodules.
    2. beta is a property of either SummaryDetail, FundPerformanceRiskOverviewStatsRow or DefaultKeyStatistics.
    3. trailingEps and forwardEps are part of DefaultKeyStatistics.

  2. As for why this is causing a compilation error for you, I need to look into it a bit more :sweat_smile: I think in the past we were very permissive about allowing users to access properties we didn't cover (i.e. in case Yahoo introduced something new, so people wouldn't need to wait on a new version every time).

    Did you ever receive any data for those fields directly from quoteSummaryResult ? If so, I need to see under which circumstances Yahoo actually provides them. Otherwise, this compilation error might actually be a big help to you :sweat_smile:

Does that make sense? What do you think?

mtorregrosa commented 1 month ago

The fields you mention were never actually "direct" fields in QuoteSummaryResult, you can see that in the original interface too (from before the recent changes). That you mentioned, they belong in:

  1. fullTimeEmployees is a property of the SummaryProfile and AssetProfile submodules.
  2. beta is a property of either SummaryDetail, FundPerformanceRiskOverviewStatsRow or DefaultKeyStatistics.
  3. trailingEps and forwardEps are part of DefaultKeyStatistics.

Yes. You are right. The types are ok. My code was wrong, but compiler does not detected it in previous versions. Now I've changed my code in accordance with the correct types. Thanks.

Another issue I've detected: Once compiled, if I run my code, I get execution errors related with the following query:

yahooFinance.historical(symbol, { events: 'dividends', period1: ..., interval: '1d' })

I get the following dividends in the result:

[ { date: 1610438400, dividends: 0.168 }, { date: 1625727600, dividends: 0.254 }, { date: 1641801600, dividends: 0.17 }, { date: 1654758000, dividends: 0.005 }, { date: 1657263600, dividends: 0.274 }, { date: 1672992000, dividends: 0.18 }, { date: 1681974000, dividends: 0.005 }, { date: 1688713200, dividends: 0.316 }, { date: 1704787200, dividends: 0.202 }, { date: 1720076400, dividends: 0.351 } ]

The problem is with the field date, that should be of type Date, in accordance with type HistoricalDividendsResult (as it was in version 2.11.3).

gadicc commented 1 month ago

@mtorregrosa, big thanks again for all this back and forth. Really appreciate it.

The problem is with the field date, that should be of type Date, in accordance with type HistoricalDividendsResult (as it was in version 2.11.3).

Absolutely right, again. This was a mistake in my convenience mapping from historical() to chart(). Thanks for raising it; fixed in 2.12.14.

And thanks re the feedback from the older release that didn't catch those issues are compile time, great that the new architecture catches these better.

mtorregrosa commented 1 month ago

Now, with version 2.12.4, it all seems to be ok for me. The software is compiled without errors and running ok.

Thank you very much.

gadicc commented 1 month ago

Thank you so much, @mtorregrosa! Appreciate all the help. :pray:

Yhprum commented 1 month ago

I seem to be missing a few types I've used in my code:

import type { CallOrPut } from "yahoo-finance2/dist/esm/src/modules/options"; import type { Option } from "yahoo-finance2/dist/esm/src/modules/options";

gadicc commented 1 month ago

Ooh, thanks, @Yhprum... somehow I missed the options module when I fixed the missing exports everywhere else (I hope! :sweat_smile:).

This is fixed now and will be in the next release (which will probably come after I get through your other issues... thanks for those too!).

gadicc commented 1 month ago

Out in 2.13.0.

mtorregrosa commented 1 month ago

I've just discovered another runtime issue that seems to be related with type validation of the response for request

  yahooFinance.quote(symbols, { fields: ['regularMarketPrice'] })

with symbols including 'BTC-EUR' (Bitcoin EUR). When I try this, I get the error

errors: [ TransformDecodeCheckError: Unable to decode value as it does not match the expected schema at Object.Decode (C:\MTM\Software\adm\node_modules\@sinclair\typebox\build\cjs\value\value\value.js:60:15) at validateAndCoerceTypebox (C:\MTM\Software\adm\node_modules\yahoo-finance2\dist\cjs\src\lib\validateAndCoerceTypes.js:60:30) at Object. (C:\MTM\Software\adm\node_modules\yahoo-finance2\dist\cjs\src\lib\moduleExec.js:98:77) at Generator.next () at fulfilled (C:\MTM\Software\adm\node_modules\yahoo-finance2\dist\cjs\src\lib\moduleExec.js:21:58) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) { schema: [Object], value: [Array], error: [Object] } ]

mtorregrosa commented 1 month ago

Previous comment is for version 2.13.0

Luminilion commented 1 month ago

Hey there, On the same subject, I've also noticed that indexTrend's type seems to not have been added as part of QuoteSummaryResultSchema. indexTrend as a module option of quoteSummary I have a ts error in my code, and then looked into yahoo-finance2/dist/esm/src/modules/quoteSummary-iface.d.ts

Any idea how to handle this ? (If I can lift some weight off your shoulders by fixing something on my end)

gadicc commented 1 month ago

@mtorregrosa:

I've just discovered another runtime issue that seems to be related with type validation of the response for request

yahooFinance.quote(symbols, { fields: ['regularMarketPrice'] })

Thanks for the report! Looking into this. I think it relates to #807 even though it's response validation. Do you get the same issue without specifying fields ? To be clear, this is definitely a bug, but it's an issue with our new validation library and I'm trying to find a chance to create a proper bug report upstream. Please let me know if removing fields helps, as it will help me prioritise this accordingly. And thanks again for reporting.

@Luminilion

indexTrend's type seems to not have been added as part of QuoteSummaryResultSchema

Right you are. Not sure how this slipped through. Re-added indexTrend and some other missing types, except, notably, insiderHolders, which looks like it will take a bit more work. Published in 2.13.1. Thanks also for your kind offer of help but this was a relatively easy fix :sweat_smile: :pray:

mtorregrosa commented 1 month ago

I've just tested that the query

yahooFinance.quote(symbols)

always works fine for me, but the query

yahooFinance.quote(symbols, { fields: ['regularMarketPrice'] })

fires validation error only if symbols include 'BTC-EUR', as stated previously.

gadicc commented 1 month ago

Gotcha; thanks, @mtorregrosa :pray: Glad you have a temporary work around so I have a bit more time to handle this properly. Have a great weekend!

gadicc commented 2 weeks ago

Hey @eddie-atkinson, moving the typebox part of our chat from #826 to here:

As you say, there were a few small things that were easily fixed. And the DX is significantly better. Unfortunately there are some other issues which haven't been as easy:

  1. Upgrading the typebox version breaks some of the types in (iirc one of) the Decode() methods. I was hoping to investigate further and create a high quality issue upstream, but haven't had a chance yet.

  2. Issues with types in unions, e.g. #807 and suspected other recent issues, including some mentioned above too. This is more serious because some related issues I found upstream said something like "we will only be able to address this in the next major version". However, it wasn't the exact issue, so maybe it could be easier. Here again, I was hoping to investigate further and open a good issue upstream but haven't had a chance.

The big question is: How much time would you potentially have to look into this now? `:) (that's an honest question, not a hint :) i.e. depending on your time and passion for this)

My other thought / option is that, with a lot of other changes coming in, we could possibly revert the typebox stuff, publish a final release for this major version, and integrate it into the next major release, which I want to start working on in the dev branch. But we should see how much work is involved in fixing things first. One other issue is:

  1. Inferred types. Although it's something that made TypeScript infinitely more useful to me, unfortunately it seems they're less appreciated now for libraries (vs your main project), because, they're slow. I'd love to get yahoo-finance2 (eventually) on JSR too (including with explicit CloudFlare support :)). You can force a package with slow types but there are implications, like those types might just be replaced with any. More details at https://jsr.io/docs/about-slow-types. I'm haven't decided how to best handle this yet (although do have some ideas) but wanted to bring it to your attention early on.

Thanks as usual for all your help and input on these matters.