Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Return For Token Authenticator #259

Merged
merged 2 commits into from
Mar 3, 2019

Conversation

jherdman
Copy link
Contributor

@jherdman jherdman commented Jan 2, 2019

I pinpointed an issue I was experiencing with the Token Authenticator
and it always returning undefined. This seemed to be something to do
with unwrapping Promise values. The makeRequest method was
unnecessarily wrapping fetch. In removing this wrapping and leaning
more on built in Promise semantics the problem went away.

I pinpointed an issue I was experiencing with the Token Authenticator
and it _always_ returning `undefined`. This seemed to be something to do
with unwrapping Promise values. The `makeRequest` method was
unnecessarily wrapping `fetch`. In removing this wrapping and leaning
more on built in Promise semantics the problem went away.
@jherdman
Copy link
Contributor Author

jherdman commented Jan 2, 2019

To clarify, this.session.authenticate(...) still returns undefined, but now I can actually access the session data.

@stale
Copy link

stale bot commented Mar 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 3, 2019
@jpadilla
Copy link
Contributor

jpadilla commented Mar 3, 2019

@fenichelar thoughts on this, at a glance it LGTM

@stale stale bot removed the wontfix label Mar 3, 2019
@fenichelar
Copy link
Owner

@jherdman Nice clean up!

@jpadilla Agreed.

I'm not sure I understand how this changes anything with regards to accessing the session data and I am able to access it as it currently stands. But this is definitely a good improvement from a simplification standpoint.

@fenichelar fenichelar merged commit 1f88b4e into fenichelar:master Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants