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

Modules return wrong type with no options (even though validateResult: true is default), unclear docs #21

Closed
pudgereyem opened this issue Feb 8, 2021 · 10 comments
Labels

Comments

@pudgereyem
Copy link
Contributor

Issue

There are multiple default exports defined for the various modules, see below:

  1. search
  2. quoteSummary
  3. quote
  4. historical
  5. autoc

I'm not sure if this is something intentional? Most of it was added in dc199b5

I noticed this because I saw that the return type for search was changed to Promise<any> from Promise<SearchResult>, which caused my app to fail since I didn't get the SearchResult type.

Proposed solution

  1. Remove all the duplicate default exports for the files above
  2. Make sure that the return type is correctly set
@pudgereyem
Copy link
Contributor Author

@gadicc I'm more than happy to create a PR for this! Just wanted to check if this was something intentional first.

@gadicc
Copy link
Owner

gadicc commented Feb 8, 2021

Hi thanks, sorry, I only just opened my email and you've already done the PR. It is intentional 😅, it's the function overloading for different arguments. If you specify { validateResult: false } we have to let typescript know that it's not getting a typesafe result anymore, but any. That's super important because we rely on typescript to confirm that our code is typesafe, so without validation it needs to enforce all the manual checks on any data we use.

You'll see the difference as soon as you change the option, e.g. if you have

const result = await yahooFinance.something('AAPL', {}, { validateResult: XXX };

If you mouseover result when XXX=true it will be a SomethingResult but if you change it to XXX=false and mouseover result you'll see it changes to any.

You can leave this issue open and I'll make this clearer in the docs. I'm out of time today though but will check back in tomorrow.

@gadicc
Copy link
Owner

gadicc commented Feb 8, 2021

PS same for the other way too:

let result : any;
try {
  // result is a SearchResult
  result = await yahooFinance.search('AAPL');
} catch (error) {
  // result is any
  result = error.result
}

because we're not sure of it's type anymore.

@pudgereyem
Copy link
Contributor Author

@gadicc, ahh I see now. So essentially for all different modules you are exporting three variants of the function:

  1. Where moduleOptions.validateResult is set to false (Will return Promise<any>)
  2. Where moduleOptions.validateResult is set to true (Will return Promise<ResponseType>)
  3. Where moduleOptions.validateResult is a boolean (Will return Promise<any>)

However, since validateResult is true by default, shouldn't it return the ResponseType in that case? The default for search is any but the default for quoteSummary is QuoteSummaryResult.

@gadicc
Copy link
Owner

gadicc commented Feb 9, 2021

However, since validateResult is true by default, shouldn't it return the ResponseType in that case? The default for search is any but the default for quoteSummary is QuoteSummaryResult.

You're absolutely right.

@gadicc gadicc closed this as completed in 1806e61 Feb 9, 2021
@gadicc
Copy link
Owner

gadicc commented Feb 9, 2021

@gadicc, ahh I see now. So essentially for all different modules you are exporting three variants of the function:

  1. Where moduleOptions.validateResult is set to false (Will return Promise<any>)
  2. Where moduleOptions.validateResult is set to true (Will return Promise<ResponseType>)
  3. Where moduleOptions.validateResult is a boolean (Will return Promise<any>)

There's actually only 1 function, your (3) above. The first two are the overloads (alternative function signatures). So (3) needs to accept the signatures for both (1) and (2) and contains the actual code. Typescript compiler will go through all signatures in order to pick the first one that matches (that's how this bug came up, because the order was wrong, they should move in increasing specificy).

Not sure how well I explained that but there's more info at https://www.typescriptlang.org/docs/handbook/functions.html#overloads.

@gadicc
Copy link
Owner

gadicc commented Feb 9, 2021

Oops I wanted this open until I improved the docs.

@gadicc gadicc reopened this Feb 9, 2021
@gadicc gadicc closed this as completed in 766aa4f Feb 9, 2021
@gadicc gadicc changed the title Multiple default exports defined in modules + wrong return type Modules return wrong type with no options (even though validateResult: true is default), unclear docs Feb 9, 2021
@pudgereyem
Copy link
Contributor Author

Morning @gadicc, you explained it very well! I read the official documentation just now as well. Thanks!

@gadicc
Copy link
Owner

gadicc commented Feb 9, 2021

You're too kind, thanks 😁

I have meetings on and off all day today but should be around here and there.

gadicc pushed a commit that referenced this issue Feb 10, 2021
# [1.7.0](v1.6.0...v1.7.0) (2021-02-10)

### Bug Fixes

* **index:** uhhhh s/_options/_opts/ like it's called everywhere else ([4492993](4492993))
* **moduleExec:** pass correct object to validation ([#27](#27)) ([8b0f9c7](8b0f9c7))
* **modules:** change overloading order specificy (fixes [#21](#21)) ([1806e61](1806e61))
* **quote:** extend marketState property ([0c36a60](0c36a60))
* **quote:** interface fixes, 10am UTC tests ([#35](#35)) ([1c256c7](1c256c7))

### Features

* new module recommendationsBySymbol ([#28](#28)) ([b467acb](b467acb))
@gadicc
Copy link
Owner

gadicc commented Feb 10, 2021

🎉 This issue has been resolved in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants