Comments (18)
Just realized Bluebird is a dependency of promisify-any. Removing this should probably come before removing Bluebird.
from node-oauth2-server.
Also here let's check if tests already cover these cases / functionality enough in order to be on the safe side. I think coverage is very high but there are a few lines missed and you know how Mr. Murphy thinks about this :-)
from node-oauth2-server.
I think we can start on this after #30 has been merged
from node-oauth2-server.
#30 is merged, we can safely start with this one :-)
from node-oauth2-server.
I'm looking into this now and noticed some oddities I thought maybe you guys could chime in on. First off, a lot of the tests are returning the wrong value, here generateAccessToken
should return a string. For example:
generateAccessToken: sinon.stub().returns({ client: {}, expiresAt: new Date(), user: {} })
should be:
generateAccessToken: sinon.stub().returns('my-access-token')
I believe these were left over from version 2 and never updated.
Second, I'm not sure util.promisify
is going to work since it requires a callback function and all the of the methods I looked at don't even have a callback as an argument. We could wrap everything in a new Promise
but that could get pretty messy.
You guys have any thoughts on this? How to promisify this:
AbstractGrantType.prototype.generateAccessToken = function(client, user, scope) {
if (this.model.generateAccessToken) {
return promisify(this.model.generateAccessToken, 3).call(this.model, client, user, scope)
.then(function(accessToken) {
return accessToken || tokenUtil.generateRandomToken();
});
}
return tokenUtil.generateRandomToken();
};
I initinally thought we could do this but it won't work since there is no callback:
AbstractGrantType.prototype.generateAccessToken = function(client, user, scope) {
if (this.model.generateAccessToken) {
return require('util').promisify(this.model.generateAccessToken)(client, user, scope)
.then(function(accessToken) {
return accessToken || tokenUtil.generateRandomToken();
});
}
return tokenUtil.generateRandomToken();
};
from node-oauth2-server.
Yes I ran the tests right now with util.promisify
and they all fail due to what you wrote @jwerre
If the functions are simple one-dimensional we could write our own wrapper like so:
const promisify = fn => function (...args) {
const env = this;
return new Promise((resolve, reject) => {
try {
resolve(fn.call(env, ...args));
} catch (e) {
reject(e);
}
});
};
I checked a few tests and it works so far but I am not sure if this works for all.
from node-oauth2-server.
Yes, that's kind of where I ended up. We could just write our own promisify function. Let me whip up a pseudo function and see what you think.
from node-oauth2-server.
@jwerre did you come up with anything? Maybe you open a draft pull request so we can work out the details using the code review?
from node-oauth2-server.
@jankapunkt I did but I don't love it. I'm not sure it's better than what is there. I'm going to test it out this morning and see if it works. Will post later today.
from node-oauth2-server.
Here... I'll just post what I have so far:
module.exports = (fn, fnArgs) => {
// I was hoping for something like this but I can't get arguments from the
// function that is passed in.
//
// let lastArg = fn.prototype.arguments.length - 1;
// let isPromise = typeof fn.prototype.arguments[lastArg] !== 'function';
// if there is no callback in the arguments, return a promise
if (!fnArgs.length || typeof fnArgs[fnArgs.length - 1] !== 'function') {
return fn;
}
// if there is a callback warp in promise
return (...args) => {
return new Promise((resolve, reject) => {
fn(...args, (err, result) => {
if (err) {
return reject(err);
}
resolve(result);
});
});
};
};
So instead of:
return promisify(this.model.generateAccessToken, 3)
...it would be:
return promisify(this.model.generateAccessToken, arguments)
I was hoping to get the arguments from the pass function but I don't think is possible so we'll have to just pass them in with the function.
from node-oauth2-server.
@jankapunkt I pushed a promisify
branch which you can take a look at. I didn't create a pull request because it's far from done and failing a bunch of test. The only files I really worked on are:
- lib/utils/promisify.js
- test/unit/utils/promisify_test.js
- lib/grant-types/abstract-grant-type.js
- test/unit/grant-types/abstract-grant-type_test.js
Running either of these test should work fine. Take a look at the code and tell me what you think.
from node-oauth2-server.
@jankapunkt @HappyZombies, what would you guys say to removing support for callbacks all together?
from node-oauth2-server.
@jwerre I am ok with that, but I believe that'll be out of scope for removing this dependency.
Going to a Promise based approach is definitely the way to go, but would require a lot of work and we need to be careful too. I think this can be something we tackle in a next major version.
We can get started on that, but I really wanna focus on getting out important fixes, administrator work NOW. If bluebird has to stay for a while then that's ok for now.
Once we get our next release out (which I am guessing will be 4.1.0), we can talk about what we want in a version 5.
If removing promisify-any is being too much of a pain, let's put a hold on it for now and get back to it later.
from node-oauth2-server.
I agree with @HappyZombies but I will still take a look into it. In the meantime we can further discuss release management
from node-oauth2-server.
I agree as well. When we deprecate callbacks we can remove Bluebird and promise-any in one fell swoop so I don't think there is any reason to spend any more time on this.
from node-oauth2-server.
@jankapunkt Are you going to take a look at this, or is this safe to close? #48
from node-oauth2-server.
@jwerre let's keep it open until we have a clear plan for #48 but once we have we can safely close this as superceded by #48
from node-oauth2-server.
I also had a look into promisify-any. promisify-any is capable of wrapping also generator functions.
Is there any necessity to support generator functions?
from node-oauth2-server.
Related Issues (20)
- Koa Wrapper for this version? HOT 5
- TypeScript rewrite HOT 6
- `validateRedirectUri` is not in the TypeScript types HOT 1
- An option to require PKCE parameters HOT 6
- Does this library support user approval dialog during authorization code grant? HOT 28
- State of this project? HOT 21
- Is implementation of `verifyScope` required? HOT 17
- generateAuthorizationCode not being awaited HOT 3
- TypeScript: Remove callback from types in 5.x HOT 4
- Move all ES5 style classes into ES6+ style class HOT 2
- getClient called with non-decoded secret/client HOT 3
- [Documentation] revokeAuthorizationCode argument should be named `code.authorizationCode`, not `code.code` HOT 4
- Client Credentials broken in 5.0.0-rc.1 HOT 12
- Insufficient integration tests HOT 3
- Contribution guidelines do not cover how to PR fixes for docs HOT 2
- wrong typing for revokeToken argument HOT 26
- PR #197 fix removed after merge HOT 3
- Typings for `validateScope` don't correctly reflect that `scope` arg can be undefined
- `authenticate` endpoint still expects `scope` as a `string` instead of `string[]` HOT 4
- PKCE Refresh token HOT 11
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from node-oauth2-server.