search

ArkeologeN / node-linkedin

LinkedIn 2.0 wrapper in Node.js
MIT License
163 stars 78 forks source link

Exchange for access token with unused parameters #59

Closed adamkchew closed 5 years ago

adamkchew commented 8 years ago 
this.getAccessToken = function (res, code, stateOut, cb) {

            if (typeof res == 'string') {
                cb = stateOut;
                stateOut = code;
                code = res;
                res = null;
            }

@ auth.js

I'm not entirely sure why we need to pass in the res since it's not being use anywhere. Ideally, I'll be function(code, state). Also I would also recommend not using parameter orders to determine your function otherwise it gets complex to debug. I can't blame the reasoning behind this because it's the nature javascript as a dynamic language (no type safety) but it'll be straight forward if all of the variables are immutable. Food for thought let me know if you want me to create a PR for this, would love to contribute.

ArkeologeN commented 8 years ago

Sound's good! Initially, it was created to support express anyway years ago. Later, been shifted to become a wide-spread npm package. That's the reason why you can see res coming in the parameters list. You can take a look over previous discussion on the same issue #14

The only idea is to not break the code anyway if anyone else is using it too. I really like the approach and would love the PR too.

What if we issue the WARN statement too so if anyone using the previous build could handle it? Cheers

ArkeologeN commented 8 years ago

Better if you write a new function retrieveAccessToken which may support Promise as well as Callback and has the similar fashion of parameters you're suggesting. We'll put the warning note with the existing function to move by the next major release.

Thoughts?