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

build(deps): upgrade react-router to v6 #1143

Merged
merged 14 commits into from
Oct 24, 2023
Merged

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Oct 21, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Related to #482
Related to #1110
Continue #726

Description of the change:

  • Upgrade react-router to v6. Reference: Official v5 to v6 Migration Guide remix-run/react-router#8753
    • Route component are now "snub"-component and are expected to be directly under Routes.
    • Update tests to render <MemoryRouter> with RouterProvider instead of using createMemoryHistory (removed in v6).
    • Note: <Promp /> is not yet supported in v6.
  • Remove yarn deps since it is not used.
  • Pin @types/react to ^17

Motivation for the change:

Upgrades to v6 in prepration to migrate to React 18. However, turn out pinning @types/react version might just solve the type issue with React upgrades as multiple installation of this package causes the issue described in #726 (comment)

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Oct 21, 2023
@tthvo tthvo added dependencies Pull requests that update a dependency file chore Refactor, rename, cleanup, etc. safe-to-test and removed needs-triage Needs thorough attention from code reviewers labels Oct 21, 2023
@tthvo tthvo marked this pull request as ready for review October 23, 2023 13:14
@tthvo tthvo requested a review from a team October 23, 2023 13:14
@andrewazores
Copy link
Member

/build_test

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1143-559418aa8920b4ea906d8d471f3210d9d565e8cc sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Oct 24, 2023

/build_test

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1143-b4cc75c67750a09d5f64ecd6b09acd394ec90161 sh smoketest.sh

@andrewazores
Copy link
Member

This is interesting:

image

It seems like now, if I kill the smoketest.sh server (presumably any Cryostat server instance) and the websocket connection to the client drops, then most views become 404s rather than showing the basic auth login. About and Settings still work, and Dashboard brings me to login, but everything else in the navigation drawer becomes a 404 when the client cannot connect to the server.

The same thing happens if I log out in the top-right user menu - most views become 404s, but I expected this instead to just display the login view everywhere instead.

I haven't tried this in an OpenShift setup to see how that auth works in a similar case.

@tthvo
Copy link
Member Author

tthvo commented Oct 24, 2023

Ahh I think that I added

<Route key={'not-found'} path={'*'} element={<PageNotFound />} />

to fall back to not-found. That would mean logging out will match only this path. Ahh, I will have a look again.

@andrewazores
Copy link
Member

I guess one possibility would be to redirect the user to / (Dashboard) before performing the actual logout call. The user would lose their state of whatever view they were on before logging out, but I don't think that would be too surprising.

@tthvo
Copy link
Member Author

tthvo commented Oct 24, 2023

Sure! How about:

<Route key={'not-found'} path={'*'} element={loggedIn? <PageNotFound />: <Login />} />

I would use Login when logging out. Haven't tested it yet but seems reasonable.

@tthvo
Copy link
Member Author

tthvo commented Oct 24, 2023

Seemed to work. Can you try again?

@tthvo
Copy link
Member Author

tthvo commented Oct 24, 2023

/build_test

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1143-caf252bc06830883b25bf7061bbb406440c2b1fc sh smoketest.sh

@andrewazores
Copy link
Member

Seems to work in smoketest.sh at least - there's a weird white flash on logout before coming back to the login screen, but I'm not too worried about that. I will try this in OpenShift as well.

@andrewazores
Copy link
Member

Worked well enough in OpenShift. There is also a white screen flash but I don't think it's a big deal. Also, I use everything in dark mode, which really emphasizes it.

@andrewazores andrewazores merged commit a011a5d into cryostatio:main Oct 24, 2023
18 checks passed
@tthvo
Copy link
Member Author

tthvo commented Oct 24, 2023

Ahh I am using light mode so I didn't notice. Interesting^^ I will keep that in mind.

@tthvo tthvo deleted the router-v6 branch October 24, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. dependencies Pull requests that update a dependency file safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants