crossplane / crossplane/function-sdk-typescript
Response helpers are inconsistent about mutate versus return
- Ngôn ngữ chính
- TypeScript
- Star
- 3
- Fork
- 2
- Merge trung bình
- 9 giờ 49 phút
- Pull request đã merge (30 ngày)
- 14
Mô tả
## 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:
```typescript
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?
```typescript
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:
```typescript
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.
Hướng dẫn đóng góp
Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này
Hướng nghiên cứu
Bắt đầu bằng cách xác định các điểm vào TypeScript cho fatal, normal, warning, setDesiredCompositeStatus, setDesiredComposedResources, setContextKey và to(), sau đó xem xét ComposeFunction (#32) cùng các bài kiểm thử của nó. Xác định liệu các helper có nhất quán trả về void hay các đối tượng mới hay không, áp dụng quyết định đó cho các chữ ký và hành vi của chúng, đồng thời thêm các bài kiểm thử bắt buộc và ghi chú phát hành cho breaking change.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Đánh giá
- Công nghệ
- typescript
- Lĩnh vực
- api, backend-api-design
- Loại issue
- Tái cấu trúc
- Độ khó
- 5/5
- Thời gian dự kiến
- Hơn một tuần
- Mức độ hoạt động
- Sôi nổi
- Độ rõ ràng
- Khá rõ ràng
- Mức phù hợp với người mới
- 35/100