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.

Sign up for GitHub

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

Jump to bottom

chore(remix-dev): remove broken param collision check #5560

Merged
merged 4 commits into from
Feb 27, 2023
Merged

chore(remix-dev): remove broken param collision check #5560

merged 4 commits into from
Feb 27, 2023

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Feb 24, 2023 •
edited
Loading

hit a snag in a demo app as it complained there was a conflict when there wasn't.

⚠️ Route Path Collision: "/:username"

The following routes all define the same URL, only the first one will be used

🟢 routes/_user.$username.tsx
⭕️️ routes/sneakers.$sneakerId.tsx

Closes: #

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 06dcb40

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

This PR includes changesets to release 18 packages
Name Type
remix Patch
@remix-run/dev Patch
create-remix Patch
@remix-run/css-bundle Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch
@remix-run/vercel Patch

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

mcansh
mcansh commented Feb 24, 2023
View reviewed changes
packages/remix-dev/__tests__/flat-routes-test.ts
@@ -619,6 +619,33 @@ describe("flatRoutes", () => {
}
});

describe("doesn't warn when there's not a route collision", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this test case is here for when param collision is brought back

@mcansh
Copy link
Collaborator Author

mcansh commented Feb 24, 2023

i think we can skip the changeset here as this was never in a stable release yet

@mcansh
chore(remix-dev): use for..of
3beb166 
Signed-off-by: Logan McAnsh <[email protected]>
@mcansh mcansh mentioned this pull request Feb 24, 2023
Closed
mcansh
mcansh commented Feb 27, 2023
View reviewed changes
packages/remix-dev/__tests__/flat-routes-test.ts
@@ -666,7 +692,7 @@ describe("flatRoutes", () => {
);
});

test("same path, different param name", () => {
test.skip("same path, different param name", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove skip once new conflict detection is brought in

@brophdawg11
Copy link
Contributor

Ah - @mcansh we will need a changeset here since this points to release-next - otherwise it won't trigger a new release. But we can remove the actual line from the final release notes since this never shipped

@mcansh
Copy link
Collaborator Author

mcansh commented Feb 27, 2023

Ah - @mcansh we will need a changeset here since this points to release-next - otherwise it won't trigger a new release. But we can remove the actual line from the final release notes since this never shipped

👍 this is the fella to remove .changeset/afraid-tigers-occur.md

@mcansh mcansh merged commit f3e0e75 into release-next Feb 27, 2023
@mcansh mcansh deleted the logan/remove-broken-param-collision branch February 27, 2023 20:00
This was referenced Feb 27, 2023
Closed
Closed
Merged
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.14.0-pre.2 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@mcansh mcansh mentioned this pull request Feb 28, 2023
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

🤖 Hello there,

We just published version 1.14.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brophdawg11 brophdawg11 brophdawg11 approved these changes

Assignees
No one assigned
Labels
CLA Signed package:dev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcansh @brophdawg11 @MichaelDeBoey