fix: TS types on Router children components

[rschristian]

This PR is a correction to the types when using components in place of the Route component as children of Router.

I added a new test which will cover the functionality that this adds. Before this change it was not possible to use anything but a type annotated const function as a child of Router. The attributes path and default would not be valid.

With this change, the following becomes viable once more:

function Home() { ... }

function App() {
  return (
    <Router>
      <Home path="/" />
    </Router>  
  );
}

[rschristian]

This is odd. Works perfectly fine in an application by fiddling with the installed module's types but here it breaks.

After reading preactjs/preact#1180 (comment) I tried bumping the TS version as it mentioned the lowest working version was 3.5.3 for a similar issue (this had 3.4.4) and that seemed to do the trick. Odd why that would make a difference, but I digress.

Edit: Though according to the package-lock.json this was using TS 3.7.5. Guess I got lucky.

I did elect to go with 4.0.x rather than 4.1.x to avoid the new JSX transforms. The latest (4.1.3) seemed to work without any issue but I'm not confident in that.

[leotaku]

I suppose this PR does not address the issue mentioned in #269. Is my assumption correct?

RoutableProps is being added to everything. This is deeply incorrect, and should be corrected as soon as possible, as it makes the types wrong. #244 is naive in suggesting this should be the case. If you need something routable, just extend or use RoutableProps as your props interface.

I personally would not have any problems with having this PR accepted, but would it maybe be possible to only add the routable properties to components that are actually the child of a Router?

This could enable the following behavior, which I consider desirable:

return (
  <div>
    <Home path="/home" /> // type error
  </div>
)

return (
  <Router>
    <Home path="/home" /> // ok
  </Router>
)

[rschristian]

@leotaku The first example should already be invalid, and I've not seen anything to the contrary.

But yes, this fixes the second case.

Edit: It looks like #269 got a fix years ago?

[leotaku]

@rschristian I'm in no way a TypeScript expert, so it's quite possible that I have misconfigured something and you are indeed correct. However, with your changes applied, I can now add the path attribute to any JSX tag anywhere without upsetting the type checker.

return (
  <div>
    <Home path="/home" /> // ok, but incorrect
  </div>
)

return (
  <Router>
    <Home path="/home" /> // ok
  </Router>
)

Edit: It looks like #269 got a fix years ago?

I don't think so. The merged PR that is visible there is on the main preactjs/preact repo and does not seem to directly solve the problems brought up in that issue.

[rschristian]

@leotaku Ah, yeah, now that you say that it rings a bell (sorry, this was made months ago).

I don't believe it's possible to add arbitrary props to children only, no. FWIW, I think this style (using <Home> as if it were a route) is a really bad practice that only works in a language as dynamic and as unstructured as JS. You probably shouldn't be using it if you're using TS. That's what <Route> is for.

This just allows you to use non-const function components as routes in TS without it throwing a fit. This just allows the same behaviour that you'd get in JS.

[leotaku]

@rschristian Yeah, that makes sense. I just thought that, if something like what I proposed were possible, this PR would be a good place to implement it. Thanks for indulging me.

[rschristian]

Definitely. And if someone knows of a way to do that, I'd love to hear it. Would be a really nice improvement.

[mx51damon]

Seems like typescript is not supported well at moment for this library ...

[rschristian]

@mx51damon It's supported, you just have to use slightly different syntax is all. preactjs-templates/typescript shows how to use TS with this library.

This PR just opens up an alternative syntax that is far less type safe and should probably be avoided if I'm being honest.

Edit: To expound, take the following:

function Home() {
    return <h1>Hello World</h1>
}

function App() {
  return (
    <Router>
      <Home path="/" />
    </Router>  
  );
}

This is perfectly fine and normal in JS land, and is what this PR allows for TS. However, what you're doing is arbitrarily adding a new prop path to a component that is specifically defined as not taking any props. Using this syntax over the <Route> component is quite odd if you're using Typescript, as you're giving up type safety for the benefits of dynamic JS.

However, it makes sense that doing the following in TS shouldn't necessarily throw an error, even if it's not type safe, as this is behavior the library supports.

[mx51damon]

preactjs-templates/typescript

Thanks man for the detailed explanation and I will give it a shot. 🙌

[mx51damon]

@mx51damon It's supported, you just have to use slightly different syntax is all. preactjs-templates/typescript shows how to use TS with this library.

This PR just opens up an alternative syntax that is far less type safe and should probably be avoided if I'm being honest.

Edit: To expound, take the following:

function Home() {
    return <h1>Hello World</h1>
}

function App() {
  return (
    <Router>
      <Home path="/" />
    </Router>  
  );
}

This is perfectly fine and normal in JS land, and is what this PR allows for TS. However, what you're doing is arbitrarily adding a new prop path to a component that is specifically defined as not taking any props. Using this syntax over the <Route> component is quite odd if you're using Typescript, as you're giving up type safety for the benefits of dynamic JS.

However, it makes sense that doing the following in TS shouldn't necessarily throw an error, even if it's not type safe, as this is behavior the library supports.

Thanks Ryan,

I think it was not working before was because of I setup my webpack dev Server in a wrong way: by using this:

static: path.resolve(PROJECT_PATH, './dist'),

And now I changed to this:

historyApiFallback: true,

All good now ~~