remix icon indicating copy to clipboard operation
remix copied to clipboard

fix: check index routes against route config instead of id suffix

Open kiliman opened this issue 2 years ago • 5 comments

Index routes in Remix Flat Routes do not end in /index so these routes were not correctly identified.

kiliman avatar Nov 10 '22 01:11 kiliman

⚠️ No Changeset found

Latest commit: 17581e50c110fa752442f59ffd5facf18048cd6e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

changeset-bot[bot] avatar Nov 10 '22 01:11 changeset-bot[bot]

@kentcdodds Yes, I'll write a test for this. I was just trying to at least get this on the radar.

In the meantime, I think I'll just update Remix Flat Routes to add the /index to the id for index routes. That way it's a non-blocker until this PR gets merged.

kiliman avatar Nov 10 '22 14:11 kiliman

Ok, v0.4.8 has been published. This updates the route ids for index routes to match what Remix is looking for. At least until Remix merges my PR.

kiliman avatar Nov 10 '22 16:11 kiliman

@kentcdodds BTW, I just realized I don't need a specific test for this, because ALL the existing tests pass. That means I didn't break anything. Just changed how the index route is determined.

kiliman avatar Nov 10 '22 18:11 kiliman

The benefit is to:

  1. Demonstrate the bug
  2. Make sure it never happens again

kentcdodds avatar Nov 10 '22 19:11 kentcdodds

Hi. I opened a PR back in May 7 that fixed this: https://github.com/remix-run/remix/pull/3127.

penspinner avatar Nov 13 '22 23:11 penspinner

@penspinner Nice.. I didn't see that one. Cool that you have tests.

BTW: it looks like there is one other place where it incorrectly checks for /index:

https://github.com/remix-run/remix/pull/4560/files?diff=unified&w=1#diff-67cd998c708c10f278fa0382338e65a4d43e3fd67e4ed5bc984d13d1d1f9725f

kiliman avatar Nov 14 '22 15:11 kiliman

@kiliman, thanks. Updated.

penspinner avatar Nov 16 '22 02:11 penspinner

I'm going to close this PR in favor of #3127.

kiliman avatar Nov 16 '22 13:11 kiliman