kiliman / remix-flat-routes

Remix package to define routes using the flat-routes convention
MIT License
695 stars 24 forks source link

Routes migration bug with hybrid convention #79

Closed elledienne closed 11 months ago

elledienne commented 11 months ago

I just run the migration script (npx migrate-flat-routes ./app/routes ./app/v2routes --convention=hybrid) on my project and realized that there seems to be something odd going on. Below are a couple of examples

Before the migration

image

After the migration

image

I'm happy to provide more info/details if helpful

kiliman commented 11 months ago

Hmm.... that does seem odd. Thanks for the example.

kiliman commented 11 months ago

Hmm... I just tested against your routes, and it looks fine. Are you sure you're on v0.6.1?

❯ tree app/routes-v1
app/routes-v1
└── resources
    ├── conversations
    │   └── $conversationId.messages.tsx
    ├── offers
    │   ├── calendar-modal.tsx
    │   ├── cancel.tsx
    │   ├── initial-state.tsx
    │   ├── new.tsx
    │   └── package-selection-grid.tsx
    ├── onboarding.tsx
    ├── openings
    │   └── new.tsx
    ├── packages
    │   └── $packageId.tsx
    ├── payment-info.tsx
    ├── products
    │   ├── channel
    │   │   └── index.tsx
    │   └── product
    │       └── index.tsx
    └── requests
        ├── $requestId.accept.tsx
        └── $requestId.reject.tsx
❯ tree app/routes-v2 
app/routes-v2
└── resources+
    ├── conversations+
    │   └── $conversationId.messages.tsx
    ├── offers+
    │   ├── calendar-modal.tsx
    │   ├── cancel.tsx
    │   ├── initial-state.tsx
    │   ├── new.tsx
    │   └── package-selection-grid.tsx
    ├── onboarding.tsx
    ├── openings+
    │   └── new.tsx
    ├── packages+
    │   └── $packageId.tsx
    ├── payment-info.tsx
    ├── products+
    │   ├── channel+
    │   │   └── _index.tsx
    │   └── product+
    │       └── _index.tsx
    └── requests+
        ├── $requestId.accept.tsx
        └── $requestId.reject.tsx
kiliman commented 11 months ago

I'm going to close this out. Reopen if you're still having issues.

elledienne commented 11 months ago

Hmm... I just tested against your routes, and it looks fine. Are you sure you're on v0.6.1?

Interesting - I also did some additional tests and realized the issue is related to the pathless layout the resources folder is in. See the full folder structure (sorry for not including it earlier)

image

I can confirm that if I move the resources folder outside the __index folder the script works as expected

kiliman commented 11 months ago

Ok thanks for providing more information. I'll take a look again.

kiliman commented 11 months ago

I was able to reproduce it, so should be able to fix the bug.

app/routes-v2
└── __index+
    ├── _layout.tsx
    ├── resources_..tsx
    ├── resources_.conversations.tsx
    ├── resources_.offers.tsx
    ├── resources_.openings.tsx
    ├── resources_.packages.tsx
    ├── resources_.products.channel.tsx
    ├── resources_.products.product.tsx
    └── resources_.requests.tsx
elledienne commented 11 months ago

I was able to reproduce it, so should be able to fix the bug.

app/routes-v2
└── __index+
    ├── _layout.tsx
    ├── resources_..tsx
    ├── resources_.conversations.tsx
    ├── resources_.offers.tsx
    ├── resources_.openings.tsx
    ├── resources_.packages.tsx
    ├── resources_.products.channel.tsx
    ├── resources_.products.product.tsx
    └── resources_.requests.tsx

I was looking into the migration script and I think 1 of the issue is related to this code

if (parentId && parentId !== 'root') {
    if (routePath?.includes('/')) {
      // multi-segment route, so need to fixup parent for flat-routes (trailing _)
      // strip parent route from route
      let currentPath = routeId.substring(parentId.length + 1)
      const [first, ...rest] = getRouteSegments(currentPath, index ?? false)
      // rewrite id to use trailing _ for parent
      routeId = `${parentId}/${first}_.${rest.join('.')}`
    }
  }

I think the check should be extended to something like parentId && parentId !== 'root' && !parentId.includes('__') to exclude pathless routes. @kiliman does that seem reasonable?

kiliman commented 11 months ago

The problem is the call to getRouteSegments(). That function is expecting the flat-routes convention, not the v1 convention, so some of the assumptions are invalid. I need to create a new version for the migration tool. I should have a fix later today.

kiliman commented 11 months ago

This has been fixed in v0.6.2