-
-
Notifications
You must be signed in to change notification settings - Fork 705
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
Feat server fn plugin #2993
base: main
Are you sure you want to change the base?
Feat server fn plugin #2993
Conversation
View your CI Pipeline Execution ↗ for commit 97af6f7.
☁️ Nx Cloud last updated this comment at |
@@ -0,0 +1,5 @@ | |||
<img src="https://static.scarf.sh/a.png?x-pxid=d988eb79-b0fc-4a2b-8514-6a1ab932d188" /> | |||
|
|||
# TanStack Start Vite Plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this was copy / pasted and needs to be updated
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/TanStack/router.git", | ||
"directory": "packages/server-functions-plugin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actual folder name is "server-functions-vite-plugin"
import { parse } from '@babel/parser' | ||
import type { ParseResult } from '@babel/parser' | ||
|
||
export type ParseAstOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should really move this thing into a common package. we have this everywhere...
"import { createClientRpc } from "my-rpc-lib"; | ||
const fn = createClientRpc({ | ||
filename: "test.ts", | ||
functionId: "test--fn_1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this have a _1
suffix, but the useServer
above does not?
filename: 'test.ts', | ||
} | ||
|
||
describe('server function compilation', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need tests for get:
and set:
?
const recordServerFunction = (nodePath: babel.NodePath) => { | ||
const fnName = findNearestVariableName(nodePath) | ||
|
||
const baseLabel = makeFileLocationUrlSafe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if you have foo.tsx
and foo.ts
? since you strip away the extension, the baseLabel could collide
42d105a
to
b1335a3
Compare
@@ -23,28 +26,44 @@ async function handleServerAction(event: H3Event) { | |||
export async function handleServerRequest(request: Request, _event?: H3Event) { | |||
const method = request.method | |||
const url = new URL(request.url, 'http://localhost:3000') | |||
// extract the serverFnId from the url as host/_server/:serverFnId | |||
// Define a regex to match the path and extract the :thing part | |||
const regex = /\/_server\/([^/?#]+)/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This base of "_server"
needs to take in the injected environment variable of "TSS_SERVER_BASE"
like its being done in packages/start/src/client-runtime/getBaseUrl.tsx
.
let action: Function | ||
if (process.env.NODE_ENV === 'development') { | ||
action = (await (globalThis as any).app | ||
.getRouter('server') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this .getRouter
call a Vinxi thing? or is it from Nitro?
Best to leave a comment here with any reference links that are available.
@@ -0,0 +1,246 @@ | |||
import { fileURLToPath, pathToFileURL } from 'node:url' | |||
|
|||
import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take inspiration from Astro here and propagate the system-reliant node modules like "fs" and "path" from the root config? https://github.com/withastro/astro/blob/98f9e833a5610cbd6f966a827451318435d31483/packages/astro/src/core/config/config.ts#L95
We've had issues opened in the past where the use of a certain system-specific node module for items like "path" and "fs" have broken things like the reading of the client-manifest.
+ for the future (if we want to), it would let us customize this or use a dependency injection strategy based on the user's system should it be required.
Also, just a preference thing on my part, perhaps, we can just rely on synchronous calls? instead of mixing between sync and async. For me, the blocking nature of sync calls makes more sense for these plugins, since they often dependant on a previous plugin/step and can be the decider for the plugin after the current one.
(() => { | ||
let serverFunctionsManifest: Record<string, DirectiveFn> | ||
return { | ||
name: 'tanstack-start-server-fn-vite-plugin-build-', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing hyphen ("-"
).
No description provided.