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

refactor(explorer): move explorer routes into its own route group #3100

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

holic
Copy link
Member

@holic holic commented Aug 30, 2024

I wanna use a new route group for some internal stuff that shouldn't inherit the base layout.tsx and apparently need to move things into route groups to "opt out" of a layout.

Copy link

changeset-bot bot commented Aug 30, 2024

⚠️ No Changeset found

Latest commit: 290c911

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

@holic
Copy link
Member Author

holic commented Aug 30, 2024

I had some concerns about using import path aliases to avoid all the ../../../.. when refactoring/moving files in VSCode, but sounds like some of those issues might be addressed in upcoming TS 5.6: microsoft/TypeScript#59119

Hopefully means we can go back to that in the future!

@holic holic marked this pull request as ready for review August 30, 2024 09:03
@holic holic requested a review from alvrs as a code owner August 30, 2024 09:03
karooolis
karooolis previously approved these changes Aug 30, 2024
@holic
Copy link
Member Author

holic commented Aug 30, 2024

needs more work for cases when you load this up with no world address

gonna defer this for now

@holic holic closed this Aug 30, 2024
@holic holic deleted the holic/explorer-route-group branch August 30, 2024 09:14
@holic holic reopened this Sep 2, 2024
@holic holic merged commit c716669 into main Sep 2, 2024
24 checks passed
const pathname = request.nextUrl.pathname;
const worldAddress = process.env.WORLD_ADDRESS;

if ((pathname === "/" || pathname === "/worlds") && worldAddress) {
Copy link
Contributor

@karooolis karooolis Sep 2, 2024

Choose a reason for hiding this comment

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

As I understand, this was probably removed in favor of redirects within pages in order not to mess with other routes that are not part of worlds/*, or are there other reasons?

Copy link
Member Author

@holic holic Sep 2, 2024

Choose a reason for hiding this comment

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

this and it helps render the styled 404 page for routes within explorer

Copy link
Contributor

@karooolis karooolis Sep 2, 2024

Choose a reason for hiding this comment

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

Which ones should valid routes moving forward, and which ones 404?

At the moment it's:

  • / -> /worlds
  • /worlds -> /worlds/${worldAddress}
  • Which are the new routes, any redirects?
  • Everything else 404s?

Just asking so I can give some more thought on how to best handle redirects.

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.

2 participants