pierrejeambrun commented on code in PR #73437:
URL: https://github.com/apache/airflow/pull/73437#discussion_r4083593130
##########
ts-sdk/package.json:
##########
@@ -69,11 +69,15 @@
"@msgpack/msgpack": "^3.1.2"
},
"peerDependencies": {
- "esbuild": "^0.28.2"
+ "esbuild": "^0.28.2",
+ "typescript": "^6.0.2"
Review Comment:
why this typescript peer dep?
##########
ts-sdk/tests/sdk/dag.test.ts:
##########
@@ -460,6 +459,84 @@ describe("Dag", () => {
expect(dag.taskIds).toEqual([]);
});
+ describe("an omitted task id", () => {
+ it("takes the handler's function name", () => {
+ const dag = new Dag("named_dag");
+ dag.task(async function extract() {})();
+
+ expect(dag.taskIds).toEqual(["extract"]);
+ });
+
+ it("takes the name of a handler declared elsewhere", () => {
+ async function transform() {}
+ const dag = new Dag("named_dag");
+ dag.task(transform)();
+
+ expect(dag.taskIds).toEqual(["transform"]);
+ });
+
+ it("still accepts a spec as the second argument", () => {
+ const dag = new Dag("named_dag");
+ dag.task(async function extract() {}, { retries: 2 })();
+
+ expect(getDagTaskRecords(dag).get("extract")?.spec).toEqual({ retries: 2
});
+ });
+
+ it("is taken from the spec when one names the task", () => {
+ const dag = new Dag("specced_id_dag");
+ dag.task(async function extract() {}, { taskId: "extract_rows" })();
+
+ expect(dag.taskIds).toEqual(["extract_rows"]);
+ });
+
+ it("prefers the positional id over the handler name", () => {
+ const dag = new Dag("positional_dag");
+ dag.task("extract_rows", async function extract() {})();
+
+ expect(dag.taskIds).toEqual(["extract_rows"]);
+ });
+
+ it("rejects a positional id and a spec id together", () => {
+ // The positional one used to win and the spec's was dropped in silence.
+ const dag = new Dag("two_ids_dag");
+
+ expect(() =>
+ dag.task("extract_rows", async function extract() {}, { taskId:
"from_spec" }),
+ ).toThrowError(
+ /Task "extract_rows" of Dag "two_ids_dag" also carries taskId
"from_spec" in its spec/,
+ );
+ expect(dag.taskIds).toEqual([]);
+ });
+
+ it("fails for an anonymous handler, naming the two ways to give it an id",
() => {
+ const dag = new Dag("anonymous_dag");
+
+ expect(() => dag.task(async () => 42)).toThrowError(
+ /A task of Dag "anonymous_dag" has no id: its handler is anonymous/,
+ );
+ expect(dag.taskIds).toEqual([]);
+ });
+
+ it("points at the packer when a handler's name was minified away", () => {
+ const dag = new Dag("minified_dag");
+ // What a bundle built without airflow-ts-pack's transform holds: the
+ // function is real, but esbuild took its name.
+ const minified = Object.defineProperty(async () => 42, "name", { value:
"" });
+
+ expect(() => dag.task(minified)).toThrowError(
+ /airflow-ts-pack resolves a named handler's id at pack time/,
+ );
+ });
Review Comment:
Why would this ever happen?
##########
ts-sdk/src/cli/pack.ts:
##########
@@ -217,10 +237,29 @@ export async function runPack(argv: readonly string[]):
Promise<void> {
target: "node22",
// A digest is only worth taking over an artifact nobody reads or edits
in place.
minify: true,
+ plugins: [taskIds.plugin],
// The manifest is read by running the staged bundle, so the metadata
describes what ships.
outfile: stagingPath,
});
+ for (const [file, ids] of taskIds.resolved) {
+ const names = ids.map(({ taskId }) => taskId).join(", ");
+ process.stderr.write(`note: ${file}: task id taken from the handler
name: ${names}\n`);
+ }
+ // A call whose first argument is not provably a handler is left as
written,
+ // and a task with no id then falls back to `handler.name` — the minified
+ // name, which changes from build to build. Said out loud, because the
+ // alternative is a task silently renamed by the bundler.
+ for (const [file, calls] of taskIds.unresolved) {
+ for (const { line, call } of calls) {
+ process.stderr.write(
+ `warning: ${file}:${line}: no task id could be read from ${call}\n` +
+ ' if this declares a task, name it — dag.task("my_task",
handler) — ' +
+ "or the minified handler name becomes the task id\n",
+ );
+ }
+ }
Review Comment:
Unresolved is only emitting a warning at pack time. Shouldn't this error out
direcly to avoid any downstream (runtime / hard to debug errors later on) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
