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

Preact-ISO: Adding TS-approved Route component #422

Merged
merged 2 commits into from Mar 12, 2021

Conversation

rschristian
Copy link
Member

Apologies in advance if this gets a bit long winded.

This PR adds a simple Route component that matches the one used in preact-router. The problem that it looks to solve is around the typings. The current support for using any component as a route brings on a few problems, namely, those route props have to be added to every single component globally and for typed components that expect route parameters, this method does not work.

For example,

const Example = (props: { id: number }) => <p>{props.id}</p>;

function App() {
  return (
    <LocationProvider>
        <Router>
          <Example path="/example/:id" /> <--- Bad, has error
          <Route path="/example/:id" component={Example} /> <--- PR content, "works"™
        </Router>
    </LocationProvider>
  );
}

The form <Example path=... /> results in the following TS error:

[tsserver 2322] [E] Type '{ path: string; }' is not assignable to type          
'IntrinsicAttributes & { id: number; }'.                                        
  Property 'id' is missing in type '{ path: string; }' but required in type     
'{ id: number; }'

Which is technically correct.

As far as I know we're the only ones still using this API. reach-router was, however, it's now being absorbed into react-router and the the community driven types decided not to support that form due to the (pretty horrible) hack needed, applying the route props globally (not ragging on anyone, I'm the one who wrote the types that allowed that; not even preact-router allows using that form in TS). Both react-router and wouter both go the way of using a <Route /> component instead, though they do so in their own ways when it comes to route parameters and the like.

This PR gives TS users the helper form <Route />, taken from preact-router. It too can arguably be called a hack, as it's essentially just dodging the type checking. I suppose it depends on people's opinions as towards how this is documented; my opinions are very TS-centric so I'd lean towards promoting this usage even if it is more verbose, as otherwise some TS users will inevitably turn up having difficulty, but on the other hand, there probably are more JS users, so this does nothing to help them.

I think it's worth exploring moving to this style of API entirely with a preact-iso v2 eventually, as TS and arbitrarily adding props/changing the types of components don't go well together. And seeing as how reach-router was merged into react-router, I'm not sure this will even remain a familiar API for users. I don't think we'd be losing out on anything, though that's a discussion for further down the road.

@changeset-bot
Copy link

changeset-bot bot commented Mar 12, 2021

🦋 Changeset detected

Latest commit: f7b5bd7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-iso Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

+1 to this. TS support is super crucial these days

@marvinhagemeister marvinhagemeister merged commit f8cae89 into main Mar 12, 2021
@marvinhagemeister marvinhagemeister deleted the feat/route-component branch March 12, 2021 09:06
@github-actions github-actions bot mentioned this pull request Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants