Skip to content
This repository has been archived by the owner on Mar 22, 2022. It is now read-only.

Decoupling #49

Merged
merged 71 commits into from
Feb 12, 2016
Merged

Decoupling #49

merged 71 commits into from
Feb 12, 2016

Conversation

ekryski
Copy link
Member

@ekryski ekryski commented Feb 12, 2016

This brings in a pretty major overhaul to feathers authentication. With it comes:

This is primarily for the oauth redirect flow.
throw new Error(`Unsupported authentication 'type': ${options.type}`);
}

// return new Promise(function(resolve, reject) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this commented out code need to kept for reference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daffl No we can get rid of it unless you want to take a crack at it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually didn't read all of it ;) What would it have to do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the promisfying we were trying to do the other night so that a developer doesn't have to call app.io.on('connect', cb) before they can authenticate. We would handle that for them. I couldn't get it to work. Too many promises and event listeners.

return app.service(options.userEndpoint).create(data, { internal: true }).then(user => {
return done(null, user);
}).catch(done);
}).catch(done);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From 60 to here it can probably just be changed to

return app.service(options.userEndpoint).create(data, { internal: true })).then(user => done(user)).catch(done);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Passport expects the first param to done to be an error or null.

@daffl
Copy link
Member

daffl commented Feb 12, 2016

Looks like it needs to be merge with master which is hopefully not too much of a disaster. Besides that, let's merge it, make a 0.2.0 release and iterate from there.

@ekryski
Copy link
Member Author

ekryski commented Feb 12, 2016

@daffl I had already did that and resolved conflicts. It's good there.

All the tests pass locally all the time. I dunno why that one test keeps timing out randomly. I can try setting the mocha timeout higher to see if that works.

@ekryski
Copy link
Member Author

ekryski commented Feb 12, 2016

never mind. The latest build did pass on all three platforms now that I added the timeout in the test. I think for some reason the tests were running fast enough that the expiredToken wasn't actually expired yet.

@ekryski
Copy link
Member Author

ekryski commented Feb 12, 2016

This PR. Closes #25, #32, #33, #38, #43, #44, #45.

ekryski added a commit that referenced this pull request Feb 12, 2016
@ekryski ekryski merged commit ebae315 into master Feb 12, 2016
@ekryski ekryski deleted the decoupling branch February 12, 2016 06:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants