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

Don't depend on React Router or Remix directly #106

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Jan 7, 2025

Closes #86

Will need to test it in an app that uses the delegated links hook to make sure passing navigate works as expected.

@charliepark and I ran into this while updating oxidecomputer/console#2576 — there was a stray remaining react-router-dom import that should have errored out, but we had a dep on react-router-dom through design-system.

<Link
to={`#${item.id}`}
<a
href={`#${item.id}`}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out these are fragment links, so there's no advantage to Link that I know of.

@david-crespo david-crespo added the minor Increment the minor version when merged label Jan 7, 2025
@david-crespo
Copy link
Collaborator Author

david-crespo commented Jan 7, 2025

Canary has the desired effect in oxidecomputer/console#2637

@benjaminleonard
Copy link
Collaborator

Sounds good. Will check it works on either the RFD site or docs. If we ever wanted to reintroduce components that had a link – for example for something like a shared dropdown link item, would we re-add the peer dep since we'd likely be using react router for everything at that point?

@david-crespo
Copy link
Collaborator Author

I guess it would be less problematic if everything was on RR 7. Another thing we might consider is breaking up this package so that the asciidoc stuff was in its own package.

@augustuswm
Copy link
Contributor

Ran in to a conflict with the peer dep version today when trying to update remix for the RFD site. It would be nice if they could be decoupled.

@steveklabnik
Copy link

Was investigating updating to rr 7, and this (depending on remix-run/react) is what will hold me back, so interested to see what yall end up choosing here :)

@david-crespo
Copy link
Collaborator Author

Will leave this as minor so we don't have too much churn on the major version. It is a breaking change to calling code that uses the delegated links hook (one repo) but shouldn't require anyone to change their deps.

@david-crespo david-crespo changed the title Don't depend on a React Router or Remix directly Don't depend on React Router or Remix directly Jan 13, 2025
@david-crespo
Copy link
Collaborator Author

I think we're gonna go with this. I have a PR to the RFD site that will make sure this change works. Unfortunately I am having trouble pushing a branch.

@david-crespo
Copy link
Collaborator Author

@david-crespo david-crespo merged commit 033f70e into master Jan 14, 2025
4 checks passed
@david-crespo david-crespo deleted the remove-rr branch January 14, 2025 00:48
@david-crespo
Copy link
Collaborator Author

@steveklabnik and @augustuswm you should be able to use @oxide/[email protected]

@steveklabnik
Copy link

worked great, thanks!

@augustuswm
Copy link
Contributor

Thanks! Will test it tomorrow

@benjaminleonard
Copy link
Collaborator

🚀 PR was released in v2.1.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on React Router
4 participants