atellmer / dark

The lightweight and powerful UI rendering engine without dependencies and written in TypeScript💫 (Browser, Node.js, Android, iOS, Windows, Linux, macOS)
MIT License
40 stars 1 forks source link

Removes trailing slashes in the router #51

Closed atellmer closed 2 months ago

atellmer commented 2 months ago

Linked issue

atellmer commented 2 months ago

it needs some tests

Coachonko commented 2 months ago

I am looking at the changes. I think const { path } = useMatch() should always return trailing slashes (consistency for developers). Maybe I am wrong here. What is the analogue behavior in the frameworks you've used before?

atellmer commented 2 months ago

I used react-router-dom before and, as far as I remember, the matched path was with a trailing slash. But on the other hand, without the trailing slash, you can write links to nested routes like this:

const { url } = useMatch();
<RouterLink to={`${url}/list`}>List</RouterLink>

instead of

<RouterLink to={`${url}list`}>List</RouterLink>

Which option is better in your opinion?

Coachonko commented 2 months ago

Not adding a slash every time is more convenient and matches the behavior of bash utilities such as pwd. Would the root path be / or in this situation? It makes more sense to me, but if the majority of users expect trailing slashes it should return trailing slashes.

I can't speak for the majority, but I do think a large amount of people use React. For sure, whichever option is chosen I do think it should be consistent: either always with trailing slashes or always without

atellmer commented 2 months ago

With current changes in the branch I see this

const routes: Routes = [
  {
    path: '/contacts', // also 'contacts' or '/contacts/'
    component: Contacts,
  },
  {
    path: '/about', // also 'about' or '/about/'
    component: About,
  },
  {
    path: '/', // also ''
    component: Home,
  },
  {
    path: '**',
    redirectTo: '/contacts', // also 'contacts' or '/contacts/'
  },
];
{path: '', url: ''} // match for home
{path: 'contacts', url: '/contacts'}  // match for contacts
{path: 'about', url: '/about'}  // match for about

Any thoughts?

If we will accept this behaviour, then I will continue dev the branch.

Coachonko commented 2 months ago

To me this seems good. Though I don't trust my opinion 😂 matchPath on wild then returns {path: '**'}? Seems good to me

So paths returned by matchPath will have both no trailing and no leading / It seems consistent, especially if the docs recommend defining routes like so:

  {
    path: 'contacts', // same as '/contacts' or '/contacts/'
    component: Contacts,
  },

Because then matchPath returns exactly what was defined ({path: 'contacts'})

This does solve the current issues.

atellmer commented 2 months ago

For wildcard ** it will be also {path: 'contacts', url: '/contacts'} because it's redirect. If you define wildcard as component, it will be {path: '**', url: '/**'} but url /** is bug apparently. But I don't know maybe it's feature? =)

Coachonko commented 2 months ago

url /** is bug apparently. But I don't know maybe it's feature? =)

I don't really know, I never matched a url because they're different on browser and server

atellmer commented 2 months ago

So, it will be feature 🫡