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

feat(remix-dev): flat routes #4880

Merged
merged 62 commits into from
Jan 5, 2023
Merged

feat(remix-dev): flat routes #4880

merged 62 commits into from
Jan 5, 2023

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Dec 15, 2022

note: this test is currently failing, but i believe that the "expected" result is actually wrong and should actually be ":$dollabills?/.lol?/what?/$?/*", so i think we have a bug in defineConventionalRoutes where this test originated from.

Closes: #

  • Docs
  • Tests

Testing Strategy:

tests and yarn playground:new

@changeset-bot
Copy link

changeset-bot bot commented Dec 15, 2022

🦋 Changeset detected

Latest commit: 3f3129e

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

This PR includes changesets to release 17 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix 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

Copy link
Collaborator

@kiliman kiliman left a comment

Choose a reason for hiding this comment

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

Hmm.. the .route suffix is actually in the original spec. Is there a reason this is removed?

Copy link
Collaborator

@kiliman kiliman left a comment

Choose a reason for hiding this comment

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

Hmm.. a lot of the code there was to make it easier to keep the enhanced version in sync with the core one. I think the enhancements will be a good proving ground for including in future core. By stripping all that out, I think it's going to be more difficult to extend in the future.

Signed-off-by: Logan McAnsh <[email protected]>

chore: use invariant instead of enforcing the type

Signed-off-by: Logan McAnsh <[email protected]>

chore: use for...of

Signed-off-by: Logan McAnsh <[email protected]>

chore: remove .route

Signed-off-by: Logan McAnsh <[email protected]>

chore: use _index for index routes

Signed-off-by: Logan McAnsh <[email protected]>

chore: replace magic characters with defined variable

Signed-off-by: Logan McAnsh <[email protected]>

fix: update default parentRoutePath

Signed-off-by: Logan McAnsh <[email protected]>

test: add test cases from old route convention

Signed-off-by: Logan McAnsh <[email protected]>

chore: update handling of special characters

Signed-off-by: Logan McAnsh <[email protected]>

chore: export route convention helpers

Signed-off-by: Logan McAnsh <[email protected]>

chore(flat-routes): add support for optional and escape route segments

Signed-off-by: Logan McAnsh <[email protected]>

chore(flat-routes): add support for optional and escape route segments

Signed-off-by: Logan McAnsh <[email protected]>
@pcattori
Copy link
Contributor

pcattori commented Dec 22, 2022

note: this test is currently failing, but i believe that the "expected" result is actually wrong and should actually be ":$dollabills?/.lol?/what?/$?/*", so i think we have a bug in defineConventionalRoutes where this test originated from.

@mcansh I has the same confusion, but I think its actually correct. But its very unintuitive. I'm thinking that we'll probably want to rethink how some of this stuff works at some point to align with intuitions a bit better.

mcansh and others added 3 commits January 5, 2023 11:40
Signed-off-by: Logan McAnsh <[email protected]>
Co-authored-by: Pedro Cattori <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Co-authored-by: Pedro Cattori <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
"parentId": undefined,
"parentId": "root",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should double-check that parentId wasn't intentionally undefined for some reason. Like does parentId get surfaced in the route meta that is available to users? Do users rely on it being undefined? Or is this value solely used for defining routes internally?

@mcansh mcansh merged commit 561796a into dev Jan 5, 2023
@mcansh mcansh deleted the logan/flat-routes branch January 5, 2023 20:25
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jan 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-45d4491-20230106 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 Jan 6, 2023
2 tasks
@manzano78
Copy link
Contributor

Hey! I tried the nightly release which includes this PR and I think I found an issue, based on the following file structure:

routes/
  app/
      _index.tsx
      primary-nav.tsx
      footer.tsx
  • footer.tsx exports a default component.

What looks wrong to me is that I can see this footer directly on screen by hitting /app/footer in the URL bar.
But if I understand the principles, only _index.tsx should be treated as a route module, right?

Before you ask, I did activate the future flag :)

@kiliman
Copy link
Collaborator

kiliman commented Jan 8, 2023

@manzano78 You should probably create a new issue instead of commenting on a closed PR.

Anyway, yes. If you are using flat-folders convention (where routes are folder names instead of filenames), then _index.tsx should be treated as the route and everything else as colocated files.

Can you run npx remix routes and paste the result? Please do so in another issue and reference this comment.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants