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

feat: forward parsePath and createPath from history #8278

Merged
merged 8 commits into from
Feb 28, 2022

Conversation

KutnerUri
Copy link
Contributor

@KutnerUri KutnerUri commented Nov 8, 2021

When using React Router, it is common to use advanced history methods, like parsePath and createPath.

React Router has a strong dependency on History, so whenever RR updates, or history releases a new version, we get errors (usually just types) and are forced to match our application History with the current version of History RR's is using. It's frustrating.
We literally just use History to use RR's features.

For this reason, it makes sense to forward History from React Router, or just the methods RR is using (parsePath, createMemoryHistory (?), createBrowserHistory, createHashHistory, createPath).
(I can make a separate PR for that, if you'd like)

It is cheap, and create a lot of value for the community.

@mjackson
Copy link
Member

mjackson commented Nov 9, 2021

We can expose parsePath and createPath if you really need them, but not the other create methods. The history library will be changing a lot in an upcoming release, and we don't want to support those methods in v6.

@KutnerUri
Copy link
Contributor Author

I see. Let me check what I am using in my code

@KutnerUri
Copy link
Contributor Author

KutnerUri commented Nov 9, 2021

This is what we use:

Values:

  • We definitely need parsePath and createPath.
  • [advanced use case] We also use createBrowserHistory() for some example page (<Router history={history}>),
    but I don't think we actually need it!

Types:

  • We use these types when forwarding props to React Router: History, Location, Query. I guess it will be good to export the placeholder types, Hash, Pathname, Search, Hash, LocationState.
  • [advanced use cases] We use these types where we listen and react for route change. (I guess we can use history specifically there.)
    History, UnregisterCallback, LocationListener, LocationDescriptor, Action

@chaance chaance changed the base branch from main to dev November 11, 2021 01:15
@ryanflorence
Copy link
Member

As Michael said, we anticipate some major changes to history and don't want to couple that API to React Router's. In v6, history even became an internal dependency.

parsePath and createPath are definitely useful in React Router though, you want to rework this to re-export those only (and whatever types are needed).

@ryanflorence ryanflorence changed the title forward history package forward parsePath and createPath from history Dec 9, 2021
@ryanflorence ryanflorence added the accepted Accepted the changes, waiting for tests/CLA/etc. label Dec 9, 2021
@KutnerUri
Copy link
Contributor Author

@ryanflorence - I updated the PR with only the relevant exports. Is this good?

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

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

@KutnerUri I think this looks good - one callout about also exporting Search, and then there's some conflicts likely due to a recent prettier change in dev. If you wouldn't mind getting those fixed up, I think this is in good shape to merge 👍

@KutnerUri
Copy link
Contributor Author

@brophdawg11 done, go ahead

@timdorr
Copy link
Member

timdorr commented Feb 27, 2022

@KutnerUri Can you also add your name to the CLA signature file? That's required for merging.

@KutnerUri
Copy link
Contributor Author

@timdorr - what is that? the contributors.yml?

@timdorr
Copy link
Member

timdorr commented Feb 28, 2022

Ah, apologies, you started this PR before the bot was in place, so it never posted its comment here. That looks like this: #8689 (comment)

But yes, you should add your username to that file.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 28, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@KutnerUri
Copy link
Contributor Author

@timdorr - any idea why this test is failing?

  ● react-router-native › re-exports createPath from react-router

    expect(received).toBe(expected) // Object.is equality

    Expected: [Function F]
    Received: undefined

I am clearly forwarding the export from packages/react-router-dom/index.tsx, what am I missing?

@KutnerUri
Copy link
Contributor Author

wait I see it..

@timdorr
Copy link
Member

timdorr commented Feb 28, 2022

There we go. Thanks!

@timdorr timdorr changed the title forward parsePath and createPath from history feat: forward parsePath and createPath from history Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the changes, waiting for tests/CLA/etc. CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants