Description
[Authored by Claude Opus 5; based on our discussion of previous work.]
Background
T404869 changed the WF* value classes away from declaring their fixed keys with
Object.defineProperty() in the constructor, to getters on the class. Two reasons: a
closure is allocated for each key of each instance, and V8 moves an object whose keys
are defined that way into dictionary mode, which slows every read of it for as long as
it lives.
T439359 then did the same for OrchestratorConfig and RequestContext, on review
feedback. Four classes outside the WF* value hierarchy were never converted and still
define their keys per instance.
Scope
| File | Keys | Instances |
|---|---|---|
| src/batching.js (Node) | labels, dependencies, state | one per ZID per batched fetch |
| src/transpilation/WFException.js | wfError | one per thrown error |
| src/Evaluator.js | programmingLanguages | one per language, per worker |
| src/memoization.js (MemoizedFunctionCache) | memoizedFunctions | one per process |
Each key already reads a this.x_ field the constructor sets, so no new backing field
is needed — the defineProperty blocks are deleted and the getters added to the class,
as in T439359's two commits.
Node and WFException are where this is worth doing: a Node is built for every ZID
of a batched fetch, and a WFException for every error raised on a request. Evaluator
and MemoizedFunctionCache are process-lifetime objects, so for those the reason is
only for CodeBase consistency.
Explicitly out of scope
Three Object.defineProperty( this, … ) calls define dynamic keys and are correct
as they are — a prototype cannot hold a key whose name is not known until runtime:
- src/transpilation/WFFunctionCall.js:36 — one key per argument name
- src/transpilation/WFObject.js:149 — one per ZObject key (a data property; see d9277fd)
- src/transpilation/WFImplementation.js:81 — one per original implementation key
Notes for whoever picks this up
- src/batching.js writes state_ through setState() (:222), so the public key stays read-only and the mutation path is unaffected. Its three getters use arrow functions (get: () => this.labels_) rather than function, but this is the instance either way, so they convert the same.
- src/transpilation/WFException.js needs the most care, because wfError is duck-typed across three unrelated classes: WFException (this accessor), EvaluatorError (WFImplementation.js:62, a plain data property) and an ad-hoc Error given the key at fetchObject.js:172. executeFunctionCall() destructures .wfError from whichever it catches, so all three must keep answering that read. Converting only WFException's to a getter leaves the other two untouched, and should stay that way.
- The semantics shift the same way each time: own, non-enumerable and non-configurable becomes non-enumerable and configurable on the prototype. Before each one, confirm as T439359 did that nothing assigns to the key and nothing reflects over the object with Object.keys(), spread, getOwnPropertyNames() or hasOwnProperty().
Acceptance criteria
- No Object.defineProperty( this, … ) remains in the four files.
- The three dynamic-key sites above are untouched.
- grep -rn "Object.defineProperty( this" src lib routes returns only those three.
- npm test passes, with no new tests needed: these are read-only keys with no behaviour of their own.
Optional, while in the area
src/transpilation/WFImplementation.js:271 calls
getFullTypeAsJSON( returnType, TypeUse.OUTPUT, this.wfConfig.requestContext ), but the
signature at src/utils.js:766 is getFullTypeAsJSON( fullType, typeUse ). The third
argument is silently ignored, presumably left from an earlier refactor; it should go.
Completion checklist
- Before closing this task, review one by one the checklist available here: https://www.mediawiki.org/wiki/Abstract_Wikipedia_team/Definition_of_Done#Back-end_Task/Bug_completion_checklist