Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Unnecessary require in passport strategies #286

Closed
ilanbiala opened this issue Dec 3, 2014 · 3 comments · Fixed by #287
Closed

Unnecessary require in passport strategies #286

ilanbiala opened this issue Dec 3, 2014 · 3 comments · Fixed by #287

Comments

@ilanbiala
Copy link
Member

The require('url') seems to be unnecessary in the passport strategies. Any reason why it's there in facebook, github, etc.?
lirantal added a commit to lirantal/meanjs that referenced this issue Dec 4, 2014
@lirantal
fixed meanjs#286 which originated due to commit meanjs@fa1171f which …
ea50c8b 
…was then re-factored to leave out the url library
@lirantal lirantal mentioned this issue Dec 4, 2014
Merged
@lirantal
Copy link
Member

lirantal commented Dec 4, 2014

Good catch @ilanbiala.

It was added by @miguelcoba in fa1171f where we needed to resolve each strategy callback URLs based on the generic appurl config option, but this has now been changed to a URL config option for each strategy so I guess this can be removed now.

PR is at #287 let me know if it missed something.

@NeverOddOrEven
Copy link
Contributor

lgtm

lirantal added a commit that referenced this issue Dec 4, 2014
@lirantal
Merge pull request #287 from lirantal/fix-unnecessary-require-url
00293bb 
Fix #286 unnecessary require url
@ilanbiala
Copy link
Member Author

Awesome.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #286 unnecessary require url lirantal/meanjs
3 participants
@lirantal @NeverOddOrEven @ilanbiala