crossplane / crossplane/function-sdk-typescript
Response helpers are inconsistent about mutate versus return
还没有人认领这个 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:
- Return void everywhere. Honest about what these do, and the smallest change in behaviour. Breaking for anyone currently chaining or reassigning.
- 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.
贡献指南
这个仓库没有索引到贡献指南
从这里开始
- 先读完整个 Issue,再读项目的贡献指南。
- 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
- Fork 仓库,在一个分支上完成修改。
- 提交 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