Closed mcsharps closed 7 years ago
I think you are correct although I haven't looked at this in quite some time.
I had originally used readsync and the file path and then switched to using environment variables because that seemed to be the best way to do it to keep my configs out of heroku, so @techieshark it appears you are correct and I'll need to put out another pr here soon to remove that line as well as update the readme to alert users to utilize a .env file instead of a data/twitter_config file but this will require another npm package like dotenv for it to read this file. Alternatively we could update the readme to tell people to manually put those environment variables when running their app e.g. TwitterConsumerKey='abc' TwitterConsumerSecret='def' node app.js. I'll await your preference and make a pr for either choice.
@mcsharps @BoyCook Hi guys, sorry but have you considered the use case of many clients with different configs? This was my case until few days ago when I restarted my server and the new version broke my app. Use environment variables is the best option, but is much better have more than one option and I think removing the config parameter hasn't been a good idea. The library's interface helps to the developer to write good code of course. but in my case, I've never hardcoded the keys in my code, always have put them in external config files. So, if someone want to put its keys hardcoded in its code, is an library responsibility avoid it? What do you think about it? May we improve this?
PS: sorry by my bad english
@pablodgonzalez After my pull was initially accepted I realized that we needed to either update the readme or add a dependency to allow something like a .env file for configuration purposes. I only added this code to give newer users who wouldn't think to keep their configurations out of github like I would when I was starting out. Honestly not sure what @BoyCook or @techieshark want as they haven't responded to my comment above. I say put out a pr if you have a different opinion for consideration or they might consider reverting this commit. I would rather do whatever is in the best interest for as many users as possible. I'll definitely still put out a pr for either of the two options I recommended if we can get some sort of consensus as to the best route to take.
@mcsharps You're right, production keys should never are in github. I think @BoyCook and @techieshark will have the same opinion. But I'm still thinking we need both options (environment variables and config object) and the config object should overwrite environment variables. I can't say what better is: if leave the pr there or put out it. Likewise a new pr is necessary for merge both config strategies. Furthermore, we need understand we are using oauth underlying, so, if someone makes the mistakes hardcoding the keys and uploading those to github, well, always can changes those on twitter so it is more annoying than dangerous. I know my case is not common, I have a multitenant app over a node cluster and use many tw apps to do the work, but I mustn't be the only one, I hope so. We should to wait feedback from the maintainers
@pablodgonzalez I absolutely agree and I highly doubt you are the only multi-tenant user out there.
per @pablodgonzalez and my conversation as well as the need to update the readme with new instructions I put in a fallback so users can use the config object as needed, but I included instructions in the readme to pref not using this method for accidentally putting keys in github. I removed all traces of using the dot env package and will leave that up to users to determine if its worth an install as to keep this library light. the pull request is here https://github.com/BoyCook/TwitterJSClient/pull/53/files
…ects, followed the pattern used in https://github.com/UnbounDev/node-strava-v3 all credit to them for the try/catch block used