Skip to content

fix: apply basePath correctly in dev mode for root index#3219

Merged
bartlomieju merged 1 commit into
freshframework:mainfrom
yukitaka:fix-basepath-root-index
Mar 29, 2026
Merged

fix: apply basePath correctly in dev mode for root index#3219
bartlomieju merged 1 commit into
freshframework:mainfrom
yukitaka:fix-basepath-root-index

Conversation

@yukitaka
Copy link
Copy Markdown
Contributor

Summary

  • Fixed an issue where basePath was not correctly applied in dev mode for the root index route
  • The dev server now uses root path "/" while preserving the configured basePath for the main app

Test plan

  • Test that routes work correctly with basePath in dev mode
  • Verify root index route loads properly with basePath configuration
  • Ensure no regression in normal dev mode without basePath
  • Ran deno task ok - all tests pass except one unrelated vite dev test that appears to be flaky

@yukitaka
Copy link
Copy Markdown
Contributor Author

I've identified the test failure. The builder_test.ts error is caused by the change in how dev config is handled. Working on a fix now.

@yukitaka
Copy link
Copy Markdown
Contributor Author

Fixed the test failures!

The solution:

  1. Set devApp's basePath to "" to simplify routing in dev mode
  2. Added displayBasePath option to ListenOptions to show the correct basePath in startup messages
  • This extends the App class's listen method, but only affects the display message
  • Alternative would be to handle this entirely within builder.ts if preferred
  1. Simplified dev middleware URL checks since basePath is always "" in dev mode
  2. Added test case to catch this issue in the future

All tests now pass. The dev server correctly handles accessing the root index at `/test` when basePath is configured.

Note: I extended ListenOptions with displayBasePath to cleanly separate the routing basePath from the display basePath. If you prefer a solution that doesn't modify the App class, I can refactor to handle this entirely within the dev builder.

@yukitaka yukitaka force-pushed the fix-basepath-root-index branch from 9b6fb92 to 2daa513 Compare August 17, 2025 17:25
@bartlomieju
Copy link
Copy Markdown
Contributor

The approach is sound — the devApp should be a transparent wrapper, and giving it basePath: "/" while letting the original appHandler handle routing avoids the double-basePath issue at the root index.

A few things:

1. displayBasePath leaks an internal concern into the public API

ListenOptions is an exported type. Adding displayBasePath for what's purely a dev builder implementation detail exposes it to all consumers. A simpler approach would be to just pass onListen directly from the builder:

await devApp.listen({
  ...options,
  onListen: createOnListen(originalBasePath, options),
});

This avoids modifying the public type entirely. createOnListen is module-scoped, but the builder lives in the same package so it could be exported internally, or the builder could construct the callback itself.

2. Middleware changes may be unnecessary

The comments in live_reload.ts and error_overlay/middleware.tsx say "In dev mode, basePath is always '/'", coupling the middleware to an assumption about how it's called. But the original code (config.basePath + DEV_ERROR_OVERLAY_URL) was actually correct generically — the real fix is in the builder setting devApp's basePath to "/". With that change, config.basePath is already "/" in these middlewares, so the prefix just becomes "/" + path which works.

Keeping the middlewares generic (config.basePath + ...) would be more robust and wouldn't require the explanatory comments.

3. Shallow copy of app.config changes sharing semantics

The current code does new App<State>(app.config) (same reference). The PR does { ...app.config, basePath: "/" } (shallow copy). Lines 195-198 then mutate devApp.config — worth verifying there's no implicit dependency on shared config mutation between the two apps.

4. Test coverage

The test checks /foo/bar (without trailing slash) but not /foo/bar/ (with trailing slash). Given this is a root index routing fix, testing both forms would be more thorough.

@yukitaka yukitaka force-pushed the fix-basepath-root-index branch from a2e8081 to 08aa8ad Compare March 26, 2026 17:47
Set devApp's basePath to "" so the dev server acts as a transparent
wrapper, letting the original appHandler handle basePath routing.
This fixes the root index route returning 404 when basePath is configured.

The onListen callback is constructed in the builder to display the
original basePath in the startup message, avoiding changes to the
public ListenOptions type.
@yukitaka yukitaka force-pushed the fix-basepath-root-index branch from 08aa8ad to f61afcb Compare March 26, 2026 18:05
@yukitaka
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review! I’ve addressed all the feedback:

  1. Removed displayBasePath from ListenOptions.
    The dev builder now directly reuses createOnListen from app.ts, so no new public option was added.

  2. Reverted middleware changes.
    live_reload.ts and error_overlay/middleware.tsx are back to the generic config.basePath + path pattern. Since devApp now runs with basePath: "", this works without special-casing.

  3. Confirmed config mutation safety with a test.
    I added an assertion in builder_test.ts to verify app.config.basePath remains "/foo/bar" after builder.listen(), confirming devApp config changes do not mutate the original app config.

  4. Trailing slash behavior is unchanged.
    /foo/bar/ returns 404 in both dev and production (including main), so this appears to be existing router behavior and likely a separate issue.

I also changed basePath: "/" to basePath: "" to align with the default and keep middleware prefix composition correct ("" + "/_frsh/alive").

Copy link
Copy Markdown
Contributor

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Thanks!

@bartlomieju bartlomieju merged commit 19d3589 into freshframework:main Mar 29, 2026
6 checks passed
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