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

defineRoute: compile-time checks #11730

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

pcattori
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Jun 25, 2024

⚠️ No Changeset found

Latest commit: b2998db

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

let route = getRoute(ctx.reactRouterConfig, id);
if (!route) return defineRoute.assertNotImported(code);

defineRoute.transform(code);
Copy link
Contributor Author

@pcattori pcattori Jun 25, 2024

Choose a reason for hiding this comment

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

In the next PR, transform will remove serverLoader + serverAction from the AST and then return the modified code to this Vite transform. In this PR, its just an assertion/compile-time check function

@pcattori pcattori force-pushed the pedro/define-route-compile-time-checks branch from 8e51540 to 07fdc1d Compare June 25, 2024 20:22
Comment on lines +7 to +8
// TODO: passing test for object literal
// TODO: passing test for `defineRoute`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add these tests in a follow-up PR since passing tests will require updates to how the route manifest is created.

if (id.endsWith(BUILD_CLIENT_ROUTE_QUERY_STRING)) return;
if (!code.includes("defineRoute")) return; // temporary back compat

if (options?.ssr) return; // TODO: why is `ctx` undefined otherwise?
Copy link
Contributor Author

@pcattori pcattori Jun 25, 2024

Choose a reason for hiding this comment

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

@markdalgleish I guess we never ran into this before just out of luck, but seems like ctx is undefined when ssr is set to true which causes a bunch of issues. Seems surprising to me, so might be missing something

not a blocker here since we only need to check these things once during client build

@pcattori pcattori force-pushed the pedro/define-route-compile-time-checks branch 2 times, most recently from 05391a4 to 6272e89 Compare June 25, 2024 21:38
@pcattori pcattori force-pushed the pedro/define-route-compile-time-checks branch from 6272e89 to b2998db Compare June 25, 2024 22:55
@pcattori pcattori merged commit ac5b36c into v7 Jun 25, 2024
7 of 8 checks passed
@pcattori pcattori deleted the pedro/define-route-compile-time-checks branch June 25, 2024 23:38
@pcattori pcattori added the v7 label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant