Skip to content

Fix nested index route imports - #312

Closed
simonyang08 wants to merge 1 commit into
laravel:mainfrom
simonyang08:codex/fix-route-index-collision
Closed

Fix nested index route imports#312
simonyang08 wants to merge 1 commit into
laravel:mainfrom
simonyang08:codex/fix-route-index-collision

Conversation

@simonyang08

Copy link
Copy Markdown

Closes #311

Summary

  • disambiguate a leaf route named index from its nested index barrel
  • import the nested barrel from ./index/index instead of self-importing ./index
  • add regression coverage for photos.index together with photos.index.window

Root cause

When both route names exist, the generated photos/index.ts barrel shares a path with the leaf route. Importing ./index therefore resolves back to the current module and silently drops the nested route helper.

User impact

Nested route helpers remain available to TypeScript and bundlers without changing generated imports for other route names.

Validation

  • vendor/bin/phpunit --filter PruneStaleFilesTest - 7 passed, 30 assertions
  • strict TypeScript compilation, esbuild, and runtime descendant assertions - passed
  • git diff --check - passed

Repository-wide formatting still reports pre-existing CRLF/style drift outside this focused change.

AI disclosure

This change was prepared with OpenAI Codex assistance and reviewed and validated locally by the contributor.

@github-actions

Copy link
Copy Markdown

Thanks for submitting a PR!

Note that draft PRs are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@simonyang08 simonyang08 changed the title [codex] Fix nested index route imports Fix nested index route imports Aug 28, 2026
@simonyang08
simonyang08 marked this pull request as ready for review September 2, 2026 03:20
@imanimen

imanimen commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Confirmed this fixes the case in #311 — I applied it to a clean main and photos.index.window resolves correctly.

There's a closely related case a few lines up that I think it misses, though. When the index prefix has no leaf route of its own:

Route::get('/albums/recent', fn () => 'ok')->name('albums.index.recent');

writeNamedFile() writes this to routes/albums/index/index.ts, so routes/albums/index.ts has no content of its own and writeBarrelFiles() takes the first branch rather than the else this PR patches. That branch still has the original filter (src/GenerateCommand.php:394 on main):

$imports = $childKeys->filter(fn ($_, $key) => $key !== 'index')->map(...)

which drops the import entirely while $childKeys still emits the assignment, leaving index undeclared:

// routes/albums/index.ts

const albums = {
    index: Object.assign(index, index),
}

export default albums

TS2304: Cannot find name 'index', and ReferenceError: index is not defined at runtime. Same root cause as #311, but a hard crash instead of a silent drop.

I verified this against your branch specifically — with this PR applied, photos.index.window passes and albums.index.recent still throws.

The filter just needs to stop dropping an index child that does have grandchildren:

$imports = $childKeys->filter(fn ($_, $key) => $key !== 'index' || $keysWithGrandkids->has($key))->map(...)

with the same ./index/index path applied here too. Keeping the drop for a grandkid-less index matters — the actions pass writes barrels before their content exists, so it always takes this branch, and widening it further would change that output.

One other thought, take it or leave it: asserting on the generated import string proves the path changed, but not that the helper actually resolves — Object.assign failing silently is what made #311 invisible to bundlers in the first place. A vitest case in the style of tests/NestedController.test.ts catches the behavior directly:

expect(photos.index.window().url).toBe("/photos/window");

#317

@simonyang08

Copy link
Copy Markdown
Author

Thanks @imanimen for checking the second case. I reviewed #317: it covers the populated-parent branch fixed here, also handles albums.index.recent when the parent has no leaf route, and adds runtime assertions for both cases.

For the maintainers: would you prefer #317 to carry the complete fix for #311, with this PR closed in its favor, or would you like the remaining case addressed here? I'm happy to defer to #317 and will hold off on overlapping changes while you choose.

@simonyang08

Copy link
Copy Markdown
Author

Thanks for the detailed reproducer. I reviewed #317 and confirmed it covers this PR's photos.index.window case as well as the additional empty-parent albums.index.recent branch, with runtime assertions for both.

To avoid duplicating the same source change, I won't push a competing expansion here while both PRs are open. I'm happy for the maintainers to choose #317 as the complete fix and close this PR, or I can update #312 if they prefer to keep the issue-linked branch. Either way, #317 should receive credit for identifying and covering the missing branch.

@joetannenbaum

Copy link
Copy Markdown
Collaborator

Thank you for this PR! I merged #317 already (just got there first) so looks like this is taken care of.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wayfinder: a route named x.index plus x.index.y silently drops y

4 participants