Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(sdk): envvars.update() ReferenceError: name is not defined outside task context#4431
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
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 | ||
| --- | ||
| Fix `envvars.update()` throwing `ReferenceError: name is not defined` when called outside a task context. The non-task branch referenced an out-of-scope `name` variable instead of the `nameOrRequestOptions` parameter (#4264). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { createServer, type Server, type ServerResponse } from "node:http"; | ||
| import type { AddressInfo } from "node:net"; | ||
| import { apiClientManager } from "@trigger.dev/core/v3"; | ||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | ||
| import * as envvars from "./envvars.js"; | ||
| type ReceivedRequest = { | ||
| method: string; | ||
| url: string; | ||
| }; | ||
| describe("envvars.update outside a task context (GH #4264)", () => { | ||
| let server: Server; | ||
| let baseUrl: string; | ||
| let requests: ReceivedRequest[]; | ||
| beforeEach(async () => { | ||
| requests = []; | ||
| server = createServer((request, response) => { | ||
| requests.push({ method: request.method ?? "", url: request.url ?? "" }); | ||
| response.writeHead(200, { "content-type": "application/json" }); | ||
| response.end(JSON.stringify({ success: true })); | ||
| }); | ||
| await new Promise<void>((resolve) => { | ||
| server.listen(0, "127.0.0.1", () => { | ||
| baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; | ||
| resolve(); | ||
| }); | ||
| }); | ||
| }); | ||
| afterEach(async () => { | ||
| await new Promise<void>((resolve) => server.close(() => resolve())); | ||
| }); | ||
| it("resolves the name argument instead of throwing ReferenceError", async () => { | ||
| const key = "tr_prod_0123456789abcdefghijklmn"; | ||
| // Outside a task context, taskContext.ctx is undefined. Before the fix this | ||
| // path evaluated `$name = name!`, but the implementation signature has no | ||
| // `name` parameter, so it threw `ReferenceError: name is not defined`. | ||
| await apiClientManager.runWithConfig({ baseURL: baseUrl, accessToken: key }, async () => { | ||
| try { | ||
| await envvars.update("proj_ref", "prod", "MY_SECRET", { value: "abc" }); | ||
| } catch (err) { | ||
| // Response-shape concerns are irrelevant here; only assert the | ||
| // argument-resolution crash is gone. | ||
| expect((err as Error).message).not.toContain("name is not defined"); | ||
| } | ||
| }); | ||
| const updateRequest = requests.find((request) => request.url.includes("MY_SECRET")); | ||
| expect(updateRequest).toBeDefined(); | ||
| expect(updateRequest!.url).toContain("/projects/proj_ref/envvars/prod/MY_SECRET"); | ||
| }); | ||
| it("throws a clear error when the name is missing", async () => { | ||
| const key = "tr_prod_0123456789abcdefghijklmn"; | ||
| await apiClientManager.runWithConfig({ baseURL: baseUrl, accessToken: key }, async () => { | ||
| await expect( | ||
| // @ts-expect-error deliberately omitting the name argument | ||
| envvars.update("proj_ref", "prod", { value: "abc" }) | ||
| ).rejects.toThrow("name is required"); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -336,9 +336,13 @@ export function update( | ||
| throw new Error("params is required"); | ||
| } | ||
| if (typeof nameOrRequestOptions !== "string") { | ||
| throw new Error("name is required"); | ||
| } | ||
| $projectRef = projectRefOrName; | ||
| $slug = slugOrParams; | ||
| $name = name!; | ||
| $name = nameOrRequestOptions; | ||
| $params = params; | ||
Comment on lines
+339
to
346
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. 🔍 Pre-existing mis-assignment in the in-task branch of update() In the Was this helpful? React with 👍 or 👎 to provide feedback. | ||
| } | ||
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.
🔍 Second test will fail: update() throws synchronously and hits the params check first
envvars.update("proj_ref", "prod", { value: "abc" })maps toprojectRefOrName="proj_ref",slugOrParams="prod",nameOrRequestOptions={value:"abc"},params=undefined. Inpackages/trigger-sdk/src/v3/envvars.ts:335-337theif (!params) throw new Error("params is required")check runs before the newly added name check, so the message is "params is required", not "name is required". Moreover the throw is synchronous (the function never returns a promise in this path), soawait expect(envvars.update(...)).rejects.toThrow(...)will not catch it — the argument expression throws while being evaluated and the test errors out. This test needsexpect(() => envvars.update(...)).toThrow(...)and a case that actually reaches the name check (e.g.update("proj_ref", "prod", undefined as any, { value: "abc" })).Was this helpful? React with 👍 or 👎 to provide feedback.