Repository navigation
Response helpers are inconsistent about mutate versus return #33
Description
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); // trueSuggested 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.resourcesPre-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.
Activity
Add a serve module so a function is one function
Add a serve module so a function is one function
Call setDesiredCompositeStatus as a statement