crossplane / crossplane/function-sdk-typescript

Response helpers are inconsistent about mutate versus return

未关闭
#33 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

还没有人认领这个 Issue。

主要语言
TypeScript
星标
3
派生
2
平均合并
9 小时 49 分钟
30 天内合并 PR
14

描述

What happened?

The response helpers are inconsistent about whether they mutate or return, and the signatures do not match the behaviour. Every helper that declares a return value returns the same object it was given — none returns a new one — so the type reads like a transformation when it is a mutation.

Measured against the current main:

helper declared actual
fatal RunFunctionResponse mutates, returns the same object
normal (inferred void) mutates, returns void
warning (inferred void) mutates, returns void
setDesiredCompositeStatus RunFunctionResponse mutates, returns the same object
setDesiredComposedResources RunFunctionResponse mutates, returns the same object
setContextKey RunFunctionResponse mutates, returns the same object

Note that fatal and its siblings normal / warning disagree with each other, so this is not even a clean split along "results helpers" versus "setters".

Why it matters more now

Under the class-based FunctionHandler, rsp was a local, so rsp = setDesiredCompositeStatus({ rsp, status }) hid the ambiguity — reassigning a local is harmless whether or not the value is new.

With ComposeFunction (#32) rsp is a parameter. Respecting the declared contract means introducing a variable:

const withStatus = setDesiredCompositeStatus({ rsp, status });
return withStatus;

…and withStatus === rsp. That variable exists only because the signature is ambiguous. Writing the honest version instead — call it, then return rsp — looks like a bug to anyone who trusts the type.

How can we reproduce it?

import { to, fatal, normal, setDesiredCompositeStatus } from '@crossplane-org/function-sdk-typescript';

const rsp = to(req);
console.log(fatal(rsp, 'm') === rsp);                                  // true
console.log(normal(rsp, 'm'));                                         // undefined
console.log(setDesiredCompositeStatus({ rsp, status: {} }) === rsp);   // true

Suggested fix

Pick one and apply it across the board:

  1. Return void everywhere. Honest about what these do, and the smallest change in behaviour. Breaking for anyone currently chaining or reassigning.
  2. Genuinely return new objects. Matches the current types, and composes nicely, but is a real behavioural change and more allocation per call.

Either is breaking, so it wants a minor version and a note in the release.

Related: to() aliases the request's desired state

Separately, to() assigns req.desired into the response rather than copying it:

const rsp = to(req);
rsp.desired === req.desired;              // true, when the request has desired state
rsp.desired.resources['x'] = something;   // also mutates req.desired.resources

Pre-existing, and mostly harmless because functions read observed state and write desired state. But ComposeFunction makes rsp.desired the primary write surface, so it is now much easier to hit. #32 documents the behaviour on ComposeFunction and pins it with a test rather than changing it, since copying is a behavioural change that belongs with the decision above.

贡献指南

这个仓库没有索引到贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

调研方向

首先定位 fatal、normal、warning、setDesiredCompositeStatus、setDesiredComposedResources、setContextKey 和 to() 的 TypeScript 入口点,然后检查 ComposeFunction (#32) 及其测试。确定这些辅助函数是否始终返回 void 或新对象,将该决定应用到它们的签名和行为中,并添加所需的测试以及针对 breaking change 的发布说明。

由索引模型根据 Issue 内容生成。

评估

技术栈
typescript
领域
api, backend-api-design
Issue 类型
重构
难度
5/5
预计耗时
一周以上
活跃度
活跃
描述清晰度
基本清楚
新手友好度
35/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。