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

[UPLOAD-1224] replace react-keycloak with react-oidc-context #1632

Merged
merged 9 commits into from
Jun 21, 2024

Conversation

krystophv
Copy link
Member

@krystophv krystophv commented May 2, 2024

For UPLOAD-1224, pretty much what it says on the tin. react-keycloak is a dead/archived project at this point, so replacing it is the prudent thing to do. Was hoping that the diff would pick up keycloak.js->auth.js as a rename, but I guess it was different enough that it treats it as a separate file. Some pieces of it got much simpler, some more complex. It still relies on keycloak.js for some specifics like generating the registration URL and logout URL (which is more relevant on blip, but for the sake of parity is also included here).

@clintonium-119 clintonium-119 self-requested a review May 6, 2024 17:01
Copy link
Member

@clintonium-119 clintonium-119 left a comment

Choose a reason for hiding this comment

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

LGTM 🔥
With the assumption that we no longer need to log errors to rollbar in auth.js, as mentioned in my comment.

app/auth.js Outdated
import * as ActionTypes from './constants/actionTypes';
import { sync, async } from './actions';
import api from '../lib/core/api';
import rollbar from './utils/rollbar';
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that rollbar is no longer used. If this is intentionally left out from the conversion from keycloak.js, then the import should likely be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, the new library doesn't really have code paths for the sort of error that we were experiencing and logging for the Keycloak version. I'll drop the import.

krystophv and others added 5 commits May 6, 2024 14:45
@gniezen
Copy link
Member

gniezen commented Jun 18, 2024

@krystophv We can merge this into the UiW branch, right?

@krystophv
Copy link
Member Author

@gniezen Yeah, will merge now.

@krystophv krystophv merged commit f8b8fed into in-browser-merged Jun 21, 2024
4 checks passed
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