Closed gadicc closed 7 years ago
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: [] }
});
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
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)
Yeah, that's the cookie bug in request-promise
I mentioned. npm i
should upgrade your installed packages to the versions in package.json
.
I tried
npm uninstall request-promise
npm i
That installed request-promise 4.2.1 but still getting the same error.
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"
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.
Ok awesome! Thanks!
That's a bit worrying that I can put "tough-cookie": "^2.3.2"
in package.json
and npm i
still leaves it at 2.3.1
! But anyways.
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!
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
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.
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?
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...
@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.
@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
@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.
Ah amazing, thanks guys!!
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.
@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.
@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)
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.
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.
@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 https://github.com/pilwon/node-yahoo-finance/pull/37/commits/3a082b68d8e9196525d48d4e941be82a8c04c94f. 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 :)
@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.
Oops I wasn't very clear :-( You can go ahead and merge.
On 24 May 2017 21:32, "Pilwon Huh" notifications@github.com wrote:
@gadicc https://github.com/gadicc I am totally fine with that approach and completely agree with your ideas around warning and README. Let me know when it's ready to be merged onto master.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pilwon/node-yahoo-finance/pull/37#issuecomment-303827872, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXUGrydf2dz3RmEPPeqp_Bhycs7PF7Oks5r9IWpgaJpZM4NeJqO .
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:
symbols
option (i.e. multiple symbols in one request - this needs multiple HTTP requests in yahoo's new API).other options:quote()
,getQuote()
,fetchQuote()
,snapshot2()
?docs, add to READMEsymbols
option (i.e. multiple symbols in one request - this needs multiple HTTP requests in yahoo's new API).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
git clone git@github.com:gadicc/node-yahoo-finance.git
-b cookie
cd node-yahoo-finance
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.
package.json
rm -rf node_modules/{tough-cookie,yahoo-finance}
npm i
(thanks sarjarapu)