Skip typegen for routes outside the app directory #12996
Conversation
🦋 Changeset detectedLatest commit: 7f1f4b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
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 |
|
Hi @wilcoxmd, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
wilcoxmd
force-pushed
the
mwilcox/skip-imported-route-typegen
branch
3 times, most recently
from
635c44c to
88c709a
Compare
Adds workspace routes fixture and a failing test
wilcoxmd
force-pushed
the
mwilcox/skip-imported-route-typegen
branch
from
88c709a to
50d0f95
Compare
| @@ -1,5 +1,5 @@ | |||
| packages: | |||
| - "integration" | |||
| - "integration/helpers/*" | |||
| - "integration/helpers/**" | |||
There was a problem hiding this comment.
This update allows the workspace fixture to install node modules in nested directories
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
wilcoxmd
force-pushed
the
mwilcox/skip-imported-route-typegen
branch
2 times, most recently
from
2132477 to
725539b
Compare
wilcoxmd
force-pushed
the
mwilcox/skip-imported-route-typegen
branch
from
725539b to
9d0dc64
Compare
| @@ -71,11 +72,21 @@ async function createContext({ | |||
| }; | |||
| } | |||
|
|
|||
| function isRouteInAppDirectory(ctx: Context, route: RouteManifestEntry) { | |||
| const absoluteRoutePath = Path.resolve(ctx.config.appDirectory, route.file); | |||
There was a problem hiding this comment.
route.file is relative to the appDirectory.
| const root = path.resolve(__dirname, "../.."); | ||
| const TMP_DIR = path.join(root, ".tmp/integration"); | ||
|
|
||
| export async function writeTestFiles( |
There was a problem hiding this comment.
This function, and the logic in createTestDirectory below were pulled from other files and just refactored into this file for reuse.
…d-route-typegen * upstream/dev: bump patch to minor for new API: `href` Type-safe href (remix-run#12994) docs: prerender/ssr:false (remix-run#13005) Skip action-only resource routes with prerender:true (remix-run#13004) Update docs for spa/prerendering Improvements to ssr:false + prerender scenarios (remix-run#12948)
This is a fix for #12993.
This PR limits typegen to only occur for route files in the current project's app directory. This behavior should be consistent with the current
getTypesPathlogic and generatedModuletyping, which also seem to assume that route files are located in the app directory.Adding this check prevents type files from being generated outside
.react-router/types(which can causetscfailures) if a route file is located outside the current project's app directory, which is the case for route definitions imported from a library or monorepo workspace package. For now, route library/workspace packages can rely on their own separate typecheck processes to ensure routes are typesafe before they are imported to another application.In the future it would be nice to figure out a way to include library route typings into a host project's typegen process. However, I imagine this approach would probably require more thought and input from the team to decide on where to generate those files and how they should be consumed. In the meantime, this PR's goal is just to help prevent unintended issues without changing the current typgen behavior too much.
Very open to feedback on this, particularly on the testing approach! I couldn't find a great pattern for testing workspace-style project structures, so I just created a new set of fixtures and a way to tweak their content as part of setting up the test in 74fed50. Let me know if you'd prefer a different strategy!