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: Load new state from session/local storage when available after a silent renew #363

Merged
merged 4 commits into from
Dec 22, 2020

Conversation

Powersource
Copy link
Contributor

Fixes #361

...I think. When I edited directly in node_modules (it complained when i npm linked because of duplicate react) it worked but this code is slightly different.

@update-docs
Copy link

update-docs bot commented Nov 26, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update some of our documentation based on your changes. You can run yarn generate-docs to generate some!

@Powersource Powersource changed the title Load new state from session/local storage when available after a silent renew fix: Load new state from session/local storage when available after a silent renew Nov 26, 2020
@Powersource
Copy link
Contributor Author

Would love some help on why those tests are failing

@Powersource
Copy link
Contributor Author

Ok managed to test locally now and seems to work :)

@simenandre
Copy link
Member

simenandre commented Nov 27, 2020

Did you manage to get them fixed? The build seems to fail, maybe you can add your fix? :)

@Powersource
Copy link
Contributor Author

No I don't have a fix for the CI stuff. My local fix was to build the package and then remove the node_modules so when I npm linked npm wouldn't get confused about the react versions. Might take a look at the CI stuff though.

@codecov
Copy link

codecov bot commented Nov 29, 2020

Codecov Report

Merging #363 (09a0bf8) into master (a6edfff) will decrease coverage by 3.77%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master     #363      +/-   ##
===========================================
- Coverage   100.00%   96.22%   -3.78%     
===========================================
  Files            1        1              
  Lines           47       53       +6     
  Branches        11       11              
===========================================
+ Hits            47       51       +4     
- Misses           0        2       +2     
Impacted Files Coverage Δ
src/AuthContext.tsx 96.22% <66.66%> (-3.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6edfff...e4aa5b2. Read the comment docs.

@Powersource
Copy link
Contributor Author

Basically just needed to update the tests so that also UserManager.events also is mocked.

@Powersource
Copy link
Contributor Author

Anything I can help with to help get this merged?

Copy link
Contributor

@ryancole ryancole left a comment

Choose a reason for hiding this comment

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

I don't think the UserManager is supposed to change, which would mean a React Effect on it wouldn't really make sense to me.

If there is an issue with the underlying oidc-client library not refreshing tokens, then perhaps this would be better as an issue on the repo for that client? That library is what handles the refreshing, as far as I can tell.

@Powersource
Copy link
Contributor Author

Powersource commented Dec 20, 2020

@ryancole

  1. Does it maybe make sense to still have the Effect to still have the removeUserLoaded if/when the AuthProvider stops being rendered but maybe due to the way an app's code is structured the UserManager object still survives?

  2. The problem isn't that oidc-client fails to refresh. It does that correctly. The problem is that this library doesn't reload the refreshed state into the userData state hook.

@ryancole
Copy link
Contributor

@Powersource Ah, OK I see now. You're right. The user data state needs to be updated when oidc-client refreshes it. That effect looks fine to me, but perhaps the dependency on userManager is not needed? Perhaps just an empty dependency array so that it sets up the event handler on first load?

In the scenario in which we use this library, our AuthProvider is at the top of our React component tree and persists through the lifetime of the app. Our UserManager never changes.

In your scenario, is UserManager changing over time?

@Powersource
Copy link
Contributor Author

Removed userManager from the dep array now :)

@ryancole
Copy link
Contributor

Looks good to me. /cc @cobraz

@simenandre simenandre merged commit 319d0e3 into bjerkio:master Dec 22, 2020
github-actions bot pushed a commit that referenced this pull request Dec 22, 2020
## [1.1.5](v1.1.4...v1.1.5) (2020-12-22)

### Bug Fixes

* Load new state from session/local storage when available after a silent renew ([#363](#363)) ([319d0e3](319d0e3))
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Powersource Powersource deleted the silent-refresh-state-reload branch December 22, 2020 21:49
@Powersource
Copy link
Contributor Author

Thanks!

@simenandre simenandre mentioned this pull request Mar 9, 2021
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.

Silent token refresh doesn't seem to actually update the token
4 participants