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

Persist credentials #50

Merged
merged 8 commits into from
Jul 13, 2016

Conversation

jlonardi
Copy link
Contributor

@jlonardi jlonardi commented Apr 28, 2016

This should fix the issues #47 and #11.

The main problem seems to be that the configuration is made with the assumption that it is used with serverside rendering. The idea seems to be that if the server invalidates credentials via TokenBridge the session would be destroyed. Since TokenBridge is not used on client-only usage the initialCredentials are allways undefined causing the call of destroying the session.

If I understood correctly from this #7 (comment), there will be full support for client-only usage. Currently client-only usage seems to work at some extend and with this fix it becomes more usable when every page reload is not anymore clearing the sessions. As said here #23 (comment) the real fix is to make a completely different configuration for client-only usage but meanwhile this should make the library a bit more usable client-only.

I noticed also that it was possible to pass storage: 'localStorage' with the settings to get the the persisting to happen in to the localStorage. This was an undocumented feature and was not sure if there is something that doesn't work when used. So far couldn't find any difference with cookies vs localstorage persiting.

This fixes also a bug where fetch was called with the saved headers as parameter instead of the api-url.

Added also an extra option so it could be possible to configure the and run the configure so that it is possible to flush credentials with client only usage.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.671% when pulling bae3962 on jlonardi:persist-credentials into 7d0875e on lynndylanhurley:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.669% when pulling cb31896 on jlonardi:persist-credentials into 7d0875e on lynndylanhurley:master.

@jlonardi
Copy link
Contributor Author

jlonardi commented Apr 28, 2016

This includes the fixes made in #48.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.669% when pulling 898d531 on jlonardi:persist-credentials into 7d0875e on lynndylanhurley:master.

@jlonardi jlonardi closed this May 2, 2016
@jlonardi jlonardi reopened this May 2, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 96.221% when pulling 2040bde on jlonardi:persist-credentials into 7d0875e on lynndylanhurley:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 96.221% when pulling 2040bde on jlonardi:persist-credentials into 7d0875e on lynndylanhurley:master.

@yiransheng
Copy link

yiransheng commented May 31, 2016

The PR will also fixes #56 .

@IstoraMandiri
Copy link
Contributor

I'm using your fork with general success. Thanks for the work.

The only problem I've found right now is that firstTimeLogin and mustResetPassword aren't triggering properly (configure.js).

I'll see if I can get this working...

@lynndylanhurley
Copy link
Owner

As said here #23 (comment) the real fix is to make a completely different configuration for client-only usage but meanwhile this should make the library a bit more usable client-only.

@jlonardi - so are you saying that this breaks server-side rendering?

@jlonardi
Copy link
Contributor Author

It should not break server-side rendering. I meant that the solution I made in this pr is not the most elegant one and might be a good idea to make totally separate configuration functions for the different configuration types (client and server-side).

@IstoraMandiri
Copy link
Contributor

@lynndylanhurley FYI I have implemented a Semantic-UI theme on top of @jlonardi's fork

The fork is working all the way up to dealing with the emailed out links; once it redirects in clientOnly mode, it's not properly updating the store

@lynndylanhurley lynndylanhurley merged commit d849631 into lynndylanhurley:master Jul 13, 2016
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.

None yet

5 participants