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

Yahoo Finance v7 API (since 2017-05-16) - initial work (1/3) #37

Merged
merged 28 commits into from
May 25, 2017

Conversation

gadicc
Copy link
Collaborator

@gadicc gadicc commented May 17, 2017

Addresses #36. WIP. Posting so it's public and obvious this is being worked on, and so others can test and comment on approach before final submission. Feedback appreciated.

Roadmap:

  • Store cookies, retrieve crumb, cache
    • Invalidate if cookie is expired
    • Invalidate if cookie has changed
    • Update cookie on each request (target download util)
  • historical()
    • Updated URLs, use v7 params
    • Get cookies/crumb before first request
    • Convert old API periods, e.g. d -> 1d
    • Ensure output is consistent with earlier API
  • snapshot()
    • Updated URLs, use v10 params
    • Get cookies/crumb before first request
    • Convert old API field options
    • Support symbols option (i.e. multiple symbols in one request - this needs multiple HTTP requests in yahoo's new API).
    • Ensure output is consistent with earlier API
    • convert dates
    • would love some help with this, since I don't use this feature myself. particularly retaining backwards compatibility with all the data retrieval options.
  • quote() - proposed new method that's closer to the current yahoo API
    • finalize name. other options: quote(), getQuote(), fetchQuote(), snapshot2() ?
    • docs, add to README
    • options sanitation
    • Support symbols option (i.e. multiple symbols in one request - this needs multiple HTTP requests in yahoo's new API).
    • convert dates!
  • Add note to README about first request taking longer & why.
  • Do it in one request. Currently, we send two requests the first time we're run. But all the data is in the original html result too, we should consider taking it just from there. Might not be worth the extra work, could save this for a later version.

Of particular note, how we retrieve the crumb, see the yahooCrumb.js source. Are we confident this will be reliable in the long term?

Trying it out / getting involved

Cloning from GitHub to play around

  1. git clone git@github.com:gadicc/node-yahoo-finance.git -b cookie
  2. cd node-yahoo-finance
  3. npm i

and use as usual.

Using in your project directly

This is dangerous. It's fine to do this to play around, but don't forget to remove it afterwards. Don't deploy with this work-in-progress code.

  1. Edit your package.json
{
  ...,
  dependencies: {
    ...,
    "yahoo-finance": "git@github.com:gadicc/node-yahoo-finance.git#cookie"
}
  1. rm -rf node_modules/{tough-cookie,yahoo-finance}
  2. npm i

(thanks sarjarapu)

@buzzcloudau
Copy link
Contributor

buzzcloudau commented May 18, 2017

Thanks for the patch. But it is returning empty quote data array for me

  yahooFinance.historical({
	  symbols: ["^VXV","XIV"],
	  from: '2017-05-05',
	  to: '2017-05-18'
	}, function (err, quotes) {

       // returns :   { '^VXV': [], XIV: [] }

 });

@gadicc
Copy link
Collaborator Author

gadicc commented May 18, 2017

Sure :) That snippet's working for me though. If you cloned from my repo, make sure you checkout the 'cookie' branch (it's not the default). If you patched an existing install, make sure you npm i again to update deps (request-promise had a cookie bug in the original version used). I added some cloning instructions to my original message to make that clearer, I guess you have to be really paying attention to notice it's not the default branch :) Are you seeing all the debug info that's still currently in the code?

fetching cookie and crumb
Cookie="B=54hc311chqlha&b=3&s=kl; Expires=Fri, 18 May 2018 08:07:06 GMT; Domain=yahoo.com; Path=/; hostOnly=?; aAge=?; cAge=2ms" 'object' true
null
{ '^VXV': 
   [ { date: 2017-05-17T04:00:00.000Z,
       open: 13.63,
       high: 15.62,
       low: 13.42,
       close: 15.6,
       adjClose: 15.6,
       volume: 0,
       symbol: '^VXV' },
...etc

@buzzcloudau
Copy link
Contributor

Ok. Thanks. I have that code running now. But hitting an error.

TypeError: str.trim is not a function
at Function.parse (/root/trader/node_modules/tough-cookie/lib/cookie.js:325:13)
at CookieJar.setCookie (/root/trader/node_modules/tough-cookie/lib/cookie.js:915:21)
at CookieJar.setCookieSync (/root/trader/node_modules/tough-cookie/lib/cookie.js:1305:18)
at RequestJar.setCookie (/root/trader/node_modules/request/lib/cookies.js:26:20)
at storeCookiesInJar (/root/trader/node_modules/yahoo-finance/lib/financeCookies.js:44:11)
at /root/trader/node_modules/yahoo-finance/lib/financeCookies.js:56:7
at tryCatcher (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/util.js:16:23)
at Promise._settlePromiseFromHandler (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/promise.js:512:31)
at Promise._settlePromise (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/promise.js:569:18)
at Promise._settlePromise0 (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/promise.js:614:10)
at Promise._settlePromises (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/promise.js:693:18)
at Async._drainQueue (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/async.js:133:16)
at Async._drainQueues (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/async.js:143:10)
at Immediate.Async.drainQueues (/root/trader/node_modules/request-promise/node_modules/bluebird/js/release/async.js:17:14)
at runCallback (timers.js:672:20)
at tryOnImmediate (timers.js:645:5)
at processImmediate [as _immediateCallback] (timers.js:617:5)

@gadicc
Copy link
Collaborator Author

gadicc commented May 18, 2017

Yeah, that's the cookie bug in request-promise I mentioned. npm i should upgrade your installed packages to the versions in package.json.

@buzzcloudau
Copy link
Contributor

buzzcloudau commented May 18, 2017

I tried

   npm uninstall request-promise
   npm i

That installed request-promise 4.2.1 but still getting the same error.

@gadicc
Copy link
Collaborator Author

gadicc commented May 18, 2017

That's weird. npm i on it's own should be enough. You can try rm -rf node_modules and then npm i to see if it makes a difference. These are the relevant versions I get:

$ grep '  "version' node_modules/{request-promise,tough-cookie}/package.json
node_modules/request-promise/package.json:  "version": "4.2.1"
node_modules/tough-cookie/package.json:  "version": "2.3.2"

@buzzcloudau
Copy link
Contributor

Whoot!

tough-cookie was 2.3.1 - but that that didnt fix it. The rm -rf node_modules did.

Thanks.

I have a couple of fixes I will submit soon. Nothing major.

@gadicc
Copy link
Collaborator Author

gadicc commented May 18, 2017

Ok awesome! Thanks!

That's a bit worrying that I can put "tough-cookie": "^2.3.2" in package.json and npm istill leaves it at 2.3.1! But anyways.

buzzcloudau and others added 4 commits May 18, 2017 21:21
- Catch exception to prevent fatal error. ( Might not be required with updated dependencies )
- Default the symbol for the url ( prevent it getting the cookie for 'undefined' )
@marcoschwartz
Copy link

Hello,

I am the founder of https://dividendstocks.io/, which completely relies on the Yahoo finance API, so currently our service is completely broken. I wanted to thank all the contributors of this package, without which our site won't even exist :) Do you know when the fix will be released? Thanks!

@sarjarapu
Copy link

sarjarapu commented May 20, 2017

Thank you @gadicc
Your commit worked for me. Here is what I did

rm -rf node_modules/

// edit package.json to make use of commit# from gadicc

"yahoo-finance": "git@github.com:gadicc/node-yahoo-finance.git#9e4f30d48b65b2c4b657d918f02b20ceecd5e2d2"

Just in case @marcoschwartz or others wants to make use your fix temporarily

@gadicc
Copy link
Collaborator Author

gadicc commented May 21, 2017

hey @marcoschwartz, your site looks awesome :) thanks, this will be my first contribution to node-yahoo-finance, so let's hope it all works out in the end :) I hope to be done by the end of this coming week depending on some possible travel, but I'd still want some proper testing after that before I submit this as final.

@sarjarapu, thanks for sharing that. I think best to use #cookie instead of the commit hash, to stay up to date, and then I guess just rm -rf node_modules/yahoo-finance && npm i to update... I think :)

Just a heads up though, this is still in development. See the Roadmap at the top for where we currently stand. I guess historical() is probably working as expected. For snapshot() it's going to take some time to see if I can get it to return results in the same format as before (so nothing breaks).

I never used snapshot() before all this went down, so not completely sure, but I think the new API actually offers a lot more data, so I might propose a snapshot2() API (or maybe just quote) which allows us to use all these extra options (@pilwon, what do you think?). See e.g. proposed quote() docs. Each key in result is an optional module.

That might also be relevant for history() - could we get stock splits before?

Other note: just added some unit and integration tests using mocha/chai/proxyquire. Still needs refining but I think it's a good foundation. The tests at least are ES7 via babel.

@marcoschwartz
Copy link

Thanks! Your latest commit worked for me, I've been able to 'patch' my app by making some changes to adapt to the new data format.

Indeed, there is much more data returned by the new version. Do you know how to actually force the snapshot function to return all this additional data? Or is there a documentation somewhere on the Yahoo side?

@gadicc
Copy link
Collaborator Author

gadicc commented May 21, 2017

Don't rely on the current output of snapshot(). It will change. I'm aiming for it to return the same data as it did before, for painless upgrades by existing authors.

But yes, I'm sure we can have a new API that's more faithful to the current Yahoo API. Check out the new quote(), which I guess is returning exactly what snapshot() gives you currently. I guess the only thing I might change (break) in the future is for it to return Date types for the dates. Name isn't final. Everything pending comments from pilwon...

TSLA quote didn't have dividend fields, added quote_MSFT.json, adding
missing dividend fields to docs/quote.md, started old API field map
in fields._map.
@gadicc
Copy link
Collaborator Author

gadicc commented May 23, 2017

@marcoschwartz, or anyone else who understands this stuff :), I'd be grateful if you could check my map at fields._map if it looks right and if you can find anything else that I missed, because, I don't work in finance, and really have no idea between the different of a forward or trailing PE / EPS, whether data is realtime (I guess it isn't, but maybe we should still return SOME result), post/pre/regular market, etc.

My last commit also added some missing fields to the quote doc (see the commit for a diff), since the TSLA example I used was missing them, and MSFT had them. If there are any more missing fields let me know.

@sarjarapu
Copy link

sarjarapu commented May 23, 2017

@marcoschwartz
See if this link helps, (not an official one). None of them are real time. PE, PS etc could be EOD. Feel free to ask any question on finance, but let's not thread crap here. 😄 YQL has better documentation but it's entirely different than what this module helps us with

@marcoschwartz
Copy link

@gadicc I had a look at the map, seems right, I only have a doubt about:

r: 'price.forwardPE', // 'PE Ratio', r2: 'price.trailingPE', // 'PE Ratio (Realtime)',

In my opinion Yahoo Finance (the website) uses the trailingPE to display the PE on the pages, so I guess r & r2 should be inverted.

@gadicc
Copy link
Collaborator Author

gadicc commented May 23, 2017

Ah amazing, thanks guys!!

@pilwon
Copy link
Owner

pilwon commented May 23, 2017

Hey @gadicc nice to see you here again :)

The addition of tests (no preference of framework), .quote() (i like this function name), and your effort to keep the API backwards compatibility are all fantastic. Thanks so much for this amazing initiative @gadicc!

One thing I'd suggest is to break this mega PR into smaller PRs so that we can make it available for all much quicker. As the recent change in Yahoo's API completely broke yahoo-finance and made the library unusable, I'd try to get the fix out there quickly and don't worry too much about potential bugs. Let's ship it once implemented (ex: .historical()) then let's improve it together as we learn along the way.

@gadicc
Copy link
Collaborator Author

gadicc commented May 23, 2017

@pilwon, hey! I actually just renamed .quote() to fetchQuote() right before I saw your message :) Hope you approve of that name too, which I thought was a bit more descriptive, but I guess matches the other APIs a bit less. Let me know if you have any further thoughts, also about the alternative API (in the docs) which I found a bit more comfortable.

Ok sure. I'm done for the day but will get on the smaller PRs tomorrow. historical() I guess is the big one and is all working (!), so we can start there. I still need to look through everything and remove some debug code etc. For snapshot() we can throw an error and say a partially backwards API is coming soon. fetchQuote() I think is actually done except for sanitation and multiple quotes. I think it's worth publishing that too since it let's people use it immediately instead of waiting for snapshot().

Everyone else, note the renaming of quote() to fetchQuote(), pending final approval from pilwon, and then it will be available via index.js too for those that want a single import. It also now returns Date objects for the dates and supports an additional API (pending pilwon's comments) - see the docs.

@pilwon
Copy link
Owner

pilwon commented May 24, 2017

@gadicc As for the naming of new function (.quote() or .fetchQuote()) I personally prefer .quote() just for the sake of consistency with others we already have. If we choose to go with .fetchQuote() or even .getQuote() I think we'll also need to change .historical() and .snapshot(), which I am totally fine as long as the users of this library want it that way. Let's get the other two merged first then we can continue this discussion with the community. (everyone -- please share your thoughts! :D)

@buzzcloudau
Copy link
Contributor

My 2c would be to continue with the current naming convention. There was a suggestion of breaking changes or enhanced features, and those could go in new functions with new the names.

And as mentioned, this library is pretty close to the google finance one. It might be a good idea to keep them as close as possible in case Time/Yahoo do something to completely block access in the future.

Disclaimer: I currently only use this library for historical data.

@gadicc
Copy link
Collaborator Author

gadicc commented May 24, 2017

Thanks guys. I agree re the name and am going to change it back :) Going to see what I can finish up today as time allows.

@gadicc
Copy link
Collaborator Author

gadicc commented May 24, 2017

@pilwon, on second thought, if shipping out a new release is the priority, let's rather merge this and I'll continue the rest as another PR. There is just so much interdependency on getCrumb, tests, utils, etc that splitting everything up will be quite complicated and take time to do so carefully and thoughtfully. Also we have to disable snapshot() and make quote() available as an alternative. Happy to have a few clean and distinct PRs but it will take more time.

I added warnings to all the methods, see 3a082b6. I think it's our responsibility for a quick interim release to make this quite prevalent. What are your thoughts? We could consider only showing each warning once though - just don't want people to miss them :) For historical() it's an error, not much else we can do here.

On that same note, see the updated README (in "Files Changed", above - it spans a few commits). I removed all the snapshot() references except one to a new snapshot() doc - don't think it makes sense to prominently mention an API which will never work as documented. And added a similar update relating to the API methods at the top. I also just saw your kind words on the current README - thanks for that :)

Let me know your thoughts. Of course I'm suggesting all these commits be squashed :) (or as mentioned, I'll make a few distinct PRs but it will take more time). Otherwise yeah I think we're ok but if you have any code of your own to test too that would be good! Ready to carry on from where I've left off after :)

@pilwon
Copy link
Owner

pilwon commented May 24, 2017

@gadicc I am totally fine with that approach and I completely agree with your ideas around deprecation warning and updates to README. Let me know when it's ready to be merged onto master.

@gadicc
Copy link
Collaborator Author

gadicc commented May 25, 2017 via email

@pilwon pilwon merged commit 901237d into pilwon:master May 25, 2017
@gadicc gadicc changed the title [WIP] Support Yahoo Finance v7 API (since 2017-05-16) Support Yahoo Finance v7 API (since 2017-05-16) - intial work May 25, 2017
@gadicc gadicc changed the title Support Yahoo Finance v7 API (since 2017-05-16) - intial work Yahoo Finance v7 API (since 2017-05-16) - initial work May 25, 2017
@gadicc gadicc changed the title Yahoo Finance v7 API (since 2017-05-16) - initial work Yahoo Finance v7 API (since 2017-05-16) - initial work (1/3) May 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants