-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(core): fallback to safe string payload in stringifyIO when superjson fails #4931
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
Changes from all commits
10ce69d
4e289b5
74db548
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@trigger.dev/sdk": patch | ||
| --- | ||
|
|
||
| `envvars.update()`: calling it outside a task run no longer throws `ReferenceError: name is not defined`. The variable name is now resolved from the positional arguments, matching the other env var methods, and a missing name raises a descriptive `name is required` error instead. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { stringifyIO } from "./ioSerialization.js"; | ||
|
|
||
| describe("stringifyIO", () => { | ||
| it("returns undefined data for undefined input", async () => { | ||
| const result = await stringifyIO(undefined); | ||
| expect(result).toEqual({ dataType: "application/json" }); | ||
| }); | ||
|
|
||
| it("returns plain text for string input", async () => { | ||
| const result = await stringifyIO("hello world"); | ||
| expect(result).toEqual({ data: "hello world", dataType: "text/plain" }); | ||
| }); | ||
|
|
||
| it("serializes normal objects using super+json", async () => { | ||
| const result = await stringifyIO({ key: "value", num: 42 }); | ||
| expect(result.dataType).toEqual("application/super+json"); | ||
| expect(typeof result.data).toBe("string"); | ||
| }); | ||
|
|
||
| it("fallback returns string data when superjson fails", async () => { | ||
| // Create an object where superjson.stringify throws or handles non-standard values | ||
| const cyclic: any = { name: "test" }; | ||
| cyclic.self = cyclic; | ||
|
|
||
| const result = await stringifyIO(cyclic); | ||
| expect(typeof result.data).toBe("string"); | ||
| }); | ||
|
Comment on lines
+21
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -96,7 +96,13 @@ export async function stringifyIO(value: any): Promise<IOPacket> { | |||||||||||||||||
|
|
||||||||||||||||||
| return { data, dataType: "application/super+json" }; | ||||||||||||||||||
| } catch { | ||||||||||||||||||
| return { data: value, dataType: "application/json" }; | ||||||||||||||||||
| try { | ||||||||||||||||||
| const data = JSON.stringify(value, makeSafeReplacer()); | ||||||||||||||||||
|
|
||||||||||||||||||
| return { data, dataType: "application/json" }; | ||||||||||||||||||
|
Comment on lines
+100
to
+102
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Undefined fallback drops payloads When fallback serialization returns Learn more
Example: A custom class instance rejected by SuperJSON can define Recommended fix: Treat an
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||
| } catch { | ||||||||||||||||||
| return { data: String(value), dataType: "text/plain" }; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,192 @@ | ||
| import { taskContext } from "@trigger.dev/core/v3"; | ||
| import { createServer, type Server } from "node:http"; | ||
| import type { AddressInfo } from "node:net"; | ||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | ||
| import { update } from "./envvars.js"; | ||
|
|
||
| describe("envvars.update outside a task context", () => { | ||
| let server: Server | undefined; | ||
| let requests: { method?: string; url?: string; body: string }[]; | ||
| let previousApiUrl: string | undefined; | ||
| let previousSecretKey: string | undefined; | ||
| let previousAccessToken: string | undefined; | ||
|
|
||
| beforeEach(async () => { | ||
| requests = []; | ||
| previousApiUrl = process.env.TRIGGER_API_URL; | ||
| previousSecretKey = process.env.TRIGGER_SECRET_KEY; | ||
| previousAccessToken = process.env.TRIGGER_ACCESS_TOKEN; | ||
|
|
||
| delete process.env.TRIGGER_SECRET_KEY; | ||
|
|
||
| server = createServer((req, res) => { | ||
| let body = ""; | ||
| req.on("data", (chunk) => { | ||
| body += chunk; | ||
| }); | ||
| req.on("end", () => { | ||
| requests.push({ method: req.method, url: req.url, body }); | ||
| res.writeHead(200, { "content-type": "application/json" }); | ||
| res.end(JSON.stringify({ success: true })); | ||
| }); | ||
| }); | ||
|
|
||
| await new Promise<void>((resolve) => server!.listen(0, "127.0.0.1", resolve)); | ||
|
|
||
| const { port } = server!.address() as AddressInfo; | ||
| process.env.TRIGGER_API_URL = `http://127.0.0.1:${port}`; | ||
| process.env.TRIGGER_ACCESS_TOKEN = "tr_test_token"; | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| if (previousApiUrl === undefined) { | ||
| delete process.env.TRIGGER_API_URL; | ||
| } else { | ||
| process.env.TRIGGER_API_URL = previousApiUrl; | ||
| } | ||
|
|
||
| if (previousSecretKey === undefined) { | ||
| delete process.env.TRIGGER_SECRET_KEY; | ||
| } else { | ||
| process.env.TRIGGER_SECRET_KEY = previousSecretKey; | ||
| } | ||
|
|
||
| if (previousAccessToken === undefined) { | ||
| delete process.env.TRIGGER_ACCESS_TOKEN; | ||
| } else { | ||
| process.env.TRIGGER_ACCESS_TOKEN = previousAccessToken; | ||
| } | ||
|
|
||
| const running = server; | ||
| server = undefined; | ||
|
|
||
| if (running) { | ||
| await new Promise<void>((resolve, reject) => | ||
| running.close((error) => (error ? reject(error) : resolve())) | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it("sends a PUT for the named variable (regression #4264)", async () => { | ||
| expect(taskContext.ctx).toBeUndefined(); | ||
|
|
||
| await expect(update("proj_xxx", "staging", "MY_VAR", { value: "hello" })).resolves.toEqual({ | ||
| success: true, | ||
| }); | ||
|
|
||
| expect(requests).toHaveLength(1); | ||
| expect(requests[0]?.method).toBe("PUT"); | ||
| expect(requests[0]?.url).toBe("/api/v1/projects/proj_xxx/envvars/staging/MY_VAR"); | ||
| expect(JSON.parse(requests[0]?.body ?? "")).toEqual({ value: "hello" }); | ||
| }); | ||
|
|
||
| it("throws a descriptive error when name is missing", () => { | ||
| expect(taskContext.ctx).toBeUndefined(); | ||
|
|
||
| expect(() => | ||
| update("proj_xxx", "staging", undefined as unknown as string, { value: "hello" }) | ||
| ).toThrow("name is required"); | ||
|
|
||
| expect(requests).toHaveLength(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("envvars.update inside a task context", () => { | ||
| let server: Server | undefined; | ||
| let requests: { method?: string; url?: string; body: string }[]; | ||
| let previousApiUrl: string | undefined; | ||
| let previousSecretKey: string | undefined; | ||
| let previousAccessToken: string | undefined; | ||
|
|
||
| beforeEach(async () => { | ||
| requests = []; | ||
| previousApiUrl = process.env.TRIGGER_API_URL; | ||
| previousSecretKey = process.env.TRIGGER_SECRET_KEY; | ||
| previousAccessToken = process.env.TRIGGER_ACCESS_TOKEN; | ||
|
|
||
| delete process.env.TRIGGER_SECRET_KEY; | ||
|
|
||
| server = createServer((req, res) => { | ||
| let body = ""; | ||
| req.on("data", (chunk) => { | ||
| body += chunk; | ||
| }); | ||
| req.on("end", () => { | ||
| requests.push({ method: req.method, url: req.url, body }); | ||
| res.writeHead(200, { "content-type": "application/json" }); | ||
| res.end(JSON.stringify({ success: true })); | ||
| }); | ||
| }); | ||
|
|
||
| await new Promise<void>((resolve) => server!.listen(0, "127.0.0.1", resolve)); | ||
|
|
||
| const { port } = server!.address() as AddressInfo; | ||
| process.env.TRIGGER_API_URL = `http://127.0.0.1:${port}`; | ||
| process.env.TRIGGER_ACCESS_TOKEN = "tr_test_token"; | ||
|
|
||
| taskContext.setGlobalLocation({ | ||
| ctx: { | ||
| project: { id: "proj_ctx_id", ref: "proj_ctx_ref", name: "Project Ctx" }, | ||
| environment: { id: "env_ctx_id", slug: "dev", type: "DEVELOPMENT" }, | ||
| organization: { id: "org_ctx_id", slug: "org_ctx", title: "Org Ctx" }, | ||
| run: { id: "run_ctx_id", isTest: false }, | ||
| task: { id: "task_ctx_id", filePath: "task.ts", exportName: "task" }, | ||
|
|
||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| taskContext.clear(); | ||
|
|
||
| if (previousApiUrl === undefined) { | ||
| delete process.env.TRIGGER_API_URL; | ||
| } else { | ||
| process.env.TRIGGER_API_URL = previousApiUrl; | ||
| } | ||
|
|
||
| if (previousSecretKey === undefined) { | ||
| delete process.env.TRIGGER_SECRET_KEY; | ||
| } else { | ||
| process.env.TRIGGER_SECRET_KEY = previousSecretKey; | ||
| } | ||
|
|
||
| if (previousAccessToken === undefined) { | ||
| delete process.env.TRIGGER_ACCESS_TOKEN; | ||
| } else { | ||
| process.env.TRIGGER_ACCESS_TOKEN = previousAccessToken; | ||
| } | ||
|
|
||
| const running = server; | ||
| server = undefined; | ||
|
|
||
| if (running) { | ||
| await new Promise<void>((resolve, reject) => | ||
| running.close((error) => (error ? reject(error) : resolve())) | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it("correctly parses explicit projectRef, slug, name parameters when taskContext exists", async () => { | ||
| await expect(update("proj_explicit", "staging", "MY_VAR", { value: "hello" })).resolves.toEqual({ | ||
| success: true, | ||
| }); | ||
|
|
||
| expect(requests).toHaveLength(1); | ||
| expect(requests[0]?.method).toBe("PUT"); | ||
| expect(requests[0]?.url).toBe("/api/v1/projects/proj_explicit/envvars/staging/MY_VAR"); | ||
| expect(JSON.parse(requests[0]?.body ?? "")).toEqual({ value: "hello" }); | ||
| }); | ||
|
|
||
| it("correctly uses taskContext defaults when only name and params are provided", async () => { | ||
| await expect(update("MY_VAR", { value: "hello_context" })).resolves.toEqual({ | ||
| success: true, | ||
| }); | ||
|
|
||
| expect(requests).toHaveLength(1); | ||
| expect(requests[0]?.method).toBe("PUT"); | ||
| expect(requests[0]?.url).toBe("/api/v1/projects/proj_ctx_ref/envvars/dev/MY_VAR"); | ||
| expect(JSON.parse(requests[0]?.body ?? "")).toEqual({ value: "hello_context" }); | ||
| }); | ||
| }); | ||
|
|
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.
🔍 Core fix lacks a changeset
The changeset covers only
@trigger.dev/sdk. The user-visible@trigger.dev/coreserialization fix receives no version bump or release note.Was this helpful? React with 👍 or 👎 to provide feedback.