search

wagenaartje / stocks.js

:moneybag: stocks.js is an easy-to-use stock market API for Javascript
https://github.com/wagenaartje/stocks.js
MIT License
326 stars 68 forks source link

use constructor not global variable for API_KEY #3

Closed kresli closed 7 years ago

kresli commented 7 years ago

I think its really bad approach to use stocks.API_KEY directly without using a constructor. What about const stock = new Stocks({API_KEY: 'xxxx'})?

wagenaartje commented 7 years ago

Coincidentally, I was thinking of something somewhat similar today. But I think your approach is best: it allows multiple Stocks objects to set up with different options (like different API keys). I'd be happy to implement this.

kresli commented 7 years ago

The main issue what I found with global variables is that test are not isolated. I'm happy to create a pull request, however the question is do you want to continue support the global variable?

wagenaartje commented 7 years ago

I think the best way to go is exactly to use new Stocks({API_KEY: 'xxxx'}). Having a global variable would only allow for requests to be made with óne API Key. I'd be glad to see the PR whenever. Please use Stocks.prototype.{function, variable} as much as possible :+1:

kresli commented 7 years ago

Why not to use a class feature?

wagenaartje commented 7 years ago

I'm really not familiar with using the class feature. I read through this question, and for me it seems like a bit of a sugar syntax. But as I stated that we should use ES6 as much as possible in the contributing guide, we should definitely use it.

I've got a couple of new simple functions in mind, so I'll wait until you created a PR with the class feature (so I become familiar with its syntax :smile:)

kresli commented 7 years ago

https://github.com/wagenaartje/stocks.js/pull/6 No, class isn't syntetic sugar anymore. If you transpiling it, then yes, however open your browser's console and create class there. You'll see thats valid. You can find all ES6 features here https://kangax.github.io/compat-table/es6/

wagenaartje commented 7 years ago

https://github.com/wagenaartje/stocks.js/commit/8548d96010f2f48dbf7bc649bf27883dd07fbc01

API has been changed to var stocks = new Stocks(apiKey). It has currently been done with Prototype, but using class is the next step.

Interesting to read through: http://wiki.c2.com/?GlobalVariablesAreBad