node-yahoo-finance2 icon indicating copy to clipboard operation
node-yahoo-finance2 copied to clipboard

Type issues since >= 2.12.0

Open gadicc opened this issue 1 year ago • 21 comments

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 avatar Sep 18 '24 09:09 gadicc

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 avatar Sep 18 '24 09:09 gadicc

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

gadicc avatar Sep 18 '24 11:09 gadicc

Thanks for the fixes in version 2.12.3:

  • Problem related with HistoricalRowDividend and HistoricalRowHistory missing types has been solved.
  • Problem related with numeric fields having type number | { raw: number } has been solved.

New compilation problem found in version 2.12.3:

  • Type QuoteSummaryResult has changed. It should have direct fields: fullTimeEmployees, beta, trailingEps, forwardEps, etc.

mtorregrosa avatar Sep 18 '24 16:09 mtorregrosa

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?

gadicc avatar Sep 19 '24 13:09 gadicc

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).

mtorregrosa avatar Sep 19 '24 16:09 mtorregrosa

@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.

gadicc avatar Sep 20 '24 12:09 gadicc

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.

mtorregrosa avatar Sep 20 '24 13:09 mtorregrosa

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

gadicc avatar Sep 20 '24 16:09 gadicc

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";

Yhprum avatar Sep 29 '24 02:09 Yhprum

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 avatar Sep 29 '24 10:09 gadicc

Out in 2.13.0.

gadicc avatar Sep 29 '24 10:09 gadicc

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 avatar Sep 30 '24 12:09 mtorregrosa

Previous comment is for version 2.13.0

mtorregrosa avatar Sep 30 '24 12:09 mtorregrosa

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

  • IndexTrendSchema and type IndexTrendSchema are correctly declared
  • but QuoteSummaryResultSchema does not seem to include an indexTrend field the same way it includes all the other modules from the doc https://github.com/gadicc/node-yahoo-finance2/blob/devel/docs/modules/quoteSummary.md

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

Luminilion avatar Oct 02 '24 09:10 Luminilion

@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:

gadicc avatar Oct 02 '24 11:10 gadicc

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.

mtorregrosa avatar Oct 02 '24 14:10 mtorregrosa

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 avatar Oct 04 '24 11:10 gadicc

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.

gadicc avatar Oct 24 '24 07:10 gadicc

@eddie-atkinson, following on from last comment last year and re the above commit:

With great regret, I decided to move back to the old approach. I definitely think that at the time we made the right call with Typebox, and the developer experience was so so so much better. However, 5 months down the line, I didn't find a good way around the above issues, and I unfortunately don't have the time to get personally involved with typebox and bug hunt or contribute changes there. Lastly, there's still the move away from using inferred types ("slow-types") for libraries.

I must still once again thank you profusely for all your research and hard work in this area. Notably, I refused to move ahead with the move back until I could solve the Cloudflare issue which started all this. After evaluating a few different options, I eventually wrote our own custom validation code. For the tiny subset of the full JSON Schema that we actually need, this still plenty fast (1,409 tests in 2.036 s on my machine), and didn't need validator runtime compilation (new Function() - the Cloudflare blocker) nor precompilation (much more work with custom types and also all the compiled validators came in at around 2mb of code).

So, I guess this solves our original problem, and we got to drop AJV as a dependency. Developer experience is worse again (for people working on the library) but user experience is better again (since all types will work properly; typescript interfaces are the single source of truth for types).

The next big thing when I have time is all the changes intended for the next major version, which I will ensure is CloudFlare compatible, if it isn't already.

Hope all is good your side and that you have a great weekend.

gadicc avatar Feb 14 '25 14:02 gadicc

HI @gadicc,

Thank you for your kind words and apologies I couldn't devote the time to help resolve the Typebox issues. Thank you for your patience and taking a chance on an unknown contributor and library. It would have been a significant time commitment on your part to research, review and improve my changes, so once again, thank you.

Also many thanks for addressing my original issue in your resolution of the problem - you didn't have to, but I very much appreciate that you did :)

I will take a look around and see if there's any spots for me to help out for the next major release. Thank you again for all your efforts

eddie-atkinson avatar Feb 15 '25 02:02 eddie-atkinson

Hey again @eddie-atkinson

Greatly appreciate your understanding on this... it wasn't a decision taken lightly. And sure, my pleasure re the validation - the least I could do, honestly.

Thanks for your desire to keep involved. I finally have a bit more time to work on all this so hope to push out a lot changes in the next few weeks, so you may want to wait for things to settle before making any further contributions otherwise please chat to me first... but, in general, your contributions are always very welcome and appreciated here 🙏

gadicc avatar Feb 15 '25 09:02 gadicc