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

Type issues since >= 2.12.0 #797

Open
gadicc opened this issue Sep 18, 2024 · 18 comments
Open

Type issues since >= 2.12.0 #797

gadicc opened this issue Sep 18, 2024 · 18 comments
Labels
bug Something isn't working

Comments

@gadicc
Copy link
Owner

gadicc commented Sep 18, 2024

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 gadicc added the bug Something isn't working label Sep 18, 2024
@gadicc
Copy link
Owner Author

gadicc commented Sep 18, 2024

As reported by @mtorregrosa in #795 (comment):

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 added a commit that referenced this issue Sep 18, 2024
With Typebox, `Static` is used to infer types from schema, however,
`StaticDecode` is needed for Transform types which have a `Decode()`
function.  This is important also for schemas composed of other
schemas that make use of transform types.

We already use `StaticDecode` in `validateAndCoerceTypes()` which
is the most important place and filters down to regular use of the
library.  But now that we are exporting all types again (per previous
commit), this is important to make sure that users who are importing
types directly get the correct type.  And I think solved some edge
cases where certain types will still coming through incorrectly.

cc: @eddie-atkinson
gadicc added a commit that referenced this issue Sep 18, 2024
* chart: Remove no-longer necessary @ts-expect-error

* options: Return a new tranformed object with correct types
  rather than mutating in place.

cc: @eddie-atkinson
gadicc pushed a commit that referenced this issue Sep 18, 2024
## [2.12.3](v2.12.2...v2.12.3) (2024-09-18)

### Bug Fixes

* **chart,options:** fix types after last commit ([#797](#797)) ([27150e6](27150e6))
* **historical:** add missing types, colocate types with schema ([#797](#797)) ([5ca349e](5ca349e))
* **types:** use StaticDecode vs Static to infer types ([#797](#797)) ([696baf6](696baf6))
@gadicc
Copy link
Owner Author

gadicc commented Sep 18, 2024

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

@mtorregrosa
Copy link

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.

@gadicc
Copy link
Owner Author

gadicc commented Sep 19, 2024

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

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 😅 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 😅

Does that make sense? What do you think?

@mtorregrosa
Copy link

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 pushed a commit that referenced this issue Sep 20, 2024
## [2.12.4](v2.12.3...v2.12.4) (2024-09-20)

### Bug Fixes

* **historical:** correctly map adjclose -> adjClose ([#795](#795)) ([d8851ec](d8851ec))
* **historical:** return coerced values ([#797](#797)) ([386bf82](386bf82))
@gadicc
Copy link
Owner Author

gadicc commented Sep 20, 2024

@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
Copy link

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
Copy link
Owner Author

gadicc commented Sep 20, 2024

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

@Yhprum
Copy link

Yhprum commented Sep 29, 2024

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
Copy link
Owner Author

gadicc commented Sep 29, 2024

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

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 pushed a commit that referenced this issue Sep 29, 2024
# [2.13.0](v2.12.5...v2.13.0) (2024-09-29)

### Bug Fixes

* **options:** re-export missing types ([#797](#797)) ([e96395f](e96395f))
* **setGlobalConfig:** cookieJar prop should be optional (fixes [#809](#809)) ([7e524fc](7e524fc))

### Features

* **quote:** export `QuoteField` type ([#808](#808)) ([a1f07d7](a1f07d7))
@gadicc
Copy link
Owner Author

gadicc commented Sep 29, 2024

Out in 2.13.0.

@mtorregrosa
Copy link

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
Copy link

Previous comment is for version 2.13.0

@Luminilion
Copy link

Luminilion commented Oct 2, 2024

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 pushed a commit that referenced this issue Oct 2, 2024
## [2.13.1](v2.13.0...v2.13.1) (2024-10-02)

### Bug Fixes

* **quoteSummary:** re-add missing modules except insiderHolders ([#797](#797)) ([d862806](d862806))
@gadicc
Copy link
Owner Author

gadicc commented Oct 2, 2024

@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 😅 🙏

@mtorregrosa
Copy link

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
Copy link
Owner Author

gadicc commented Oct 4, 2024

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

@gadicc
Copy link
Owner Author

gadicc commented Oct 24, 2024

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. I get an error when querying { fields } in .quotes. #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.

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

4 participants