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: setup react-router compatibility package #6285

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Oct 27, 2024

Summary

Instead of attempting to upgrade react-router to v6 and update all of our code simultaneously we'll use the backwards compatibility package to allow for a gradual transition.

Upgrading strategy -> https://www.notion.so/Upgrading-react-router-to-v6-12cc8e7d1f5c80a3b66ad474d7478050#4b3251525fa144e69d5442ec7aaa9bad

Related Issues / PR's

close #6167

Screenshots

NA

Affected Areas and Manually Tested Areas


Important

Add react-router-dom-v5-compat and use CompatRouter for gradual upgrade to react-router v6.

  • Dependencies:
    • Add react-router-dom-v5-compat to package.json for gradual upgrade to react-router v6.
  • Routing:
    • Use CompatRouter in index.tsx to maintain compatibility with react-router v5 while preparing for v6.

This description was created by Ellipsis for e9e29d1. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to def66a6 in 23 seconds

More details
  • Looked at 108 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/AppRoutes/index.tsx:242
  • Draft comment:
    Consider removing the Router from react-router-dom if CompatRouter from react-router-dom-v5-compat is intended to replace it. This will avoid redundancy and potential confusion.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment addresses a potential redundancy in the code where both 'Router' and 'CompatRouter' are used. If 'CompatRouter' is meant to replace 'Router', then having both could indeed be redundant and confusing. The comment seems to be about a change made in the diff, specifically the addition of 'CompatRouter'.
    I might be missing the context of why both 'Router' and 'CompatRouter' are used together. There could be a specific reason for this setup that the comment does not account for.
    The comment is suggesting a code change to avoid redundancy, which is a valid concern. If there is no specific reason for using both, the comment is helpful.
    Keep the comment as it suggests a valid code change to avoid redundancy and potential confusion.
2. frontend/src/AppRoutes/index.tsx:240
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable to the PrivateRoute and routes imports.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_gZTSyQ0G5oV1mAzD


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ahmadshaheer ahmadshaheer changed the title chore: setup router compatibility package chore: setup react-router compatibility package Oct 27, 2024
@YounixM YounixM force-pushed the chore/add-react-router-compatibility-package branch from def66a6 to e9e29d1 Compare October 28, 2024 05:56
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e9e29d1 in 21 seconds

More details
  • Looked at 108 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/AppRoutes/index.tsx:242
  • Draft comment:
    Consider removing the Router component from react-router-dom as CompatRouter should handle routing. Double-check if CompatRouter requires Router as a parent component.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative, asking to double-check if CompatRouter requires Router as a parent. This goes against the rules of not making speculative comments or asking the author to double-check. Without strong evidence that CompatRouter can replace Router, the comment should be removed.
    I might be missing specific knowledge about CompatRouter and its requirements. If CompatRouter is indeed a drop-in replacement for Router, the comment could be valid.
    The comment does not provide strong evidence or certainty about the need to remove Router. It is speculative and asks for verification, which is against the rules.
    Remove the comment as it is speculative and asks for verification without providing strong evidence.
2. frontend/src/AppRoutes/index.tsx:240
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable to the AppRoutes and Private imports.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_7oBv7U227EmBUAYB


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@YounixM
Copy link
Member

YounixM commented Oct 28, 2024

@ahmadshaheer : Requested notion doc access.

@ahmadshaheer ahmadshaheer merged commit dc5917d into develop Nov 7, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update react-router-dom
3 participants