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

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

Closed
wants to merge 1 commit into from

Conversation

kiliman
Copy link
Collaborator

@kiliman kiliman commented Nov 10, 2022

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

Index routes in Remix Flat Routes do not end in `/index` so these routes were not correctly identified.
@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2022

⚠️ No Changeset found

Latest commit: 17581e5

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

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Any chance we could get a test for this?

@kiliman
Copy link
Collaborator Author

kiliman commented Nov 10, 2022

@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 added a commit to kiliman/remix-flat-routes that referenced this pull request Nov 10, 2022
@kiliman
Copy link
Collaborator Author

kiliman commented Nov 10, 2022

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
Copy link
Collaborator Author

kiliman commented Nov 10, 2022

@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.

@kentcdodds
Copy link
Member

The benefit is to:

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

@penspinner
Copy link
Contributor

penspinner commented Nov 13, 2022

Hi. I opened a PR back in May 7 that fixed this: #3127.

@kiliman
Copy link
Collaborator Author

kiliman commented Nov 14, 2022

@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

@penspinner
Copy link
Contributor

@kiliman, thanks. Updated.

@kiliman
Copy link
Collaborator Author

kiliman commented Nov 16, 2022

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

@kiliman kiliman closed this Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants