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

chore(router): replace react-router-dom v6 with react-router-dom-v5-compat #1529

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

aptmac
Copy link
Member

@aptmac aptmac commented Jan 9, 2025

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


Fixes: #1528

Description of the change:

This change replaces the current react-router-dom dependency (and its imports) with react-router-dom-v5-compat

Motivation for the change:

The react-router-dom-v5-compat pack contains the APIs for both v5 and v6 of the router, so no functional changes are required. This change is necessary for the Console Plugin, which only has react-router-dom v5.3.x and react-router-dom-v5-compat v6.11.2 as shared modules. This change allows for the pages that contain useNavigate to render without errors in the Console.

As mentioned in the issue I've linked, there is (were) intentions to update the Console to provide react-router v6 to dynamic plugins, and when that time comes this PR can just be undone.

@aptmac aptmac requested a review from a team as a code owner January 9, 2025 17:55
@aptmac aptmac added the chore Refactor, rename, cleanup, etc. label Jan 9, 2025
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Jan 9, 2025
@andrewazores
Copy link
Member

andrewazores commented Jan 9, 2025

Needs rebasing, and unit tests need to be updated since there is some interaction between Jest and the router module. But looks good otherwise.

@andrewazores andrewazores added safe-to-test and removed needs-triage Needs thorough attention from code reviewers labels Jan 9, 2025
@aptmac aptmac force-pushed the react-router-dom-v5-compat branch from bad3591 to bc2611c Compare January 9, 2025 20:07
@aptmac
Copy link
Member Author

aptmac commented Jan 9, 2025

The tests were running fine locally, but in the CI complained of not being able to find 'react-router-dom'. Adding it back at v5.3.x similar to how the console frontend has it looks to have done the trick: b103623#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R116

@andrewazores
Copy link
Member

Seems strange. I wonder if that means that somewhere there's a transitive dependency on a react-router-dom component that the test harness is trying to load, and if we can instead add another jest mock/stub or requireActual to force it to only use the compat version instead?

@aptmac
Copy link
Member Author

aptmac commented Jan 9, 2025

I'm wondering now if maybe it's my own setup is doing funny things..

Re-reading the v5-compat instructions it mentions to run it in parallel with v5, and it's package.json has a peer dependency on either v4 or v5 of react-router-dom.

@aptmac aptmac force-pushed the react-router-dom-v5-compat branch from b103623 to 2bc37cf Compare January 14, 2025 16:15
@aptmac aptmac force-pushed the react-router-dom-v5-compat branch from 2bc37cf to be14c7c Compare January 14, 2025 16:15
@andrewazores andrewazores merged commit 2a0601d into cryostatio:main Jan 14, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants