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

New frontend example for react-router #529

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jimmyt857
Copy link

@jimmyt857 jimmyt857 commented Nov 27, 2024

With the recent merge between React Router and Remix, React Router v7 is a compelling option as a frontend framework and I wanted to try it out. Sending a PR because I thought others might benefit from my attempt at getting this working within Bazel!

Although the dev and production versions of the server are both working, this example is 95% of the way there. Unfortunately I'm not yet deeply familiar with rules_js, rules_ts, or even react-router. I need a bit of help fixing the //react-router/app:app_typecheck and //react-router/app:test tests, they are each failing for a different reason:

  • :app_typecheck fails because it is missing some special typing files generated by react-router. See their documentation here. I already wrapped the react-router typegen command into a js_run_binary but I couldn't figure out how to make the ts_library target accept the generated files as input.
  • :test fails because the vite library somehow doesn't contain expect. Honestly not sure why this is happening, the test code is taken almost exactly from https://github.com/bazelbuild/examples/blob/main/frontend/react/src/App.test.tsx.

@jimmyt857 jimmyt857 requested a review from alexeagle as a code owner November 27, 2024 07:41
//
// Normally, this file is optional (https://reactrouter.com/explanation/special-files#entryservertsx) and react-router would use the copy in its own package as the default.
// However, when run in Bazel it is not able to find the default copy at runtime.
// TODO: Figure out how to make this work in Bazel.

Choose a reason for hiding this comment

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

what's broken here?

Copy link
Author

Choose a reason for hiding this comment

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

The "broken" behavior is the fact that this file must be vendored into our repo.

The default behavior of react router is to fallback to its own internal copy of this file, when this file is omitted. Somehow though, when running in bazel, the fallback fails. I'm copying the fallback file into my own repo so that react router can find it.

Copy link

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

It looks good to me.

Copy link
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

I'll pull this down and try it out, maybe I can see what needs fixing and/or add some automated testing so the PR would be red

@alexeagle alexeagle force-pushed the 11-26-new_frontend_example_for_react-router branch from 29fb9e2 to d3f6389 Compare January 31, 2025 23:22
@alexeagle
Copy link
Contributor

I checked in rules_ts and actually we do have a very example for combining folders with rootDirs.
https://github.com/aspect-build/rules_ts/tree/main/examples/root_dirs
This can work here as well. The key is that the ts_project has to be at the common ancestor folder of the rootDirs, since we set rootDir to control output locations. For some reason this morning I'm getting a permission denied pushing to your branch so I've put a commit to illustrate over here: main...alexeagle:examples:11-26-new_frontend_example_for_react-router

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.

3 participants