SOF-8032: convert ide to TypeScript, make ComputedEntityMix… - #22
Open
k0stik wants to merge 8 commits into
Open
Conversation
k0stik
force-pushed
the
chore/SOF-8032
branch
2 times, most recently
from
August 20, 2026 16:19
9864917 to
a02d972
Compare
…in generic [release]
ide was the last package in the stack still built with babel from plain JS,
with its public types supplied by a hand-written, never-typechecked
compute.d.ts. ComputedEntityMixin declared `readonly compute: unknown`,
which collides with every consumer's generated schema mixin also declaring
a real `compute` (TS2320 requires identical types across multiple extends) -
forcing wode's Workflow.ts and jode's Job.ts to redeclare `compute` with a
`// TODO: fix ComputedEntityMixin and remove this`, and wode's Subworkflow.ts
to `Omit<ComputedEntityMixin, "compute">` with no explanation.
- Convert src/js/*.js -> *.ts, delete the hand-written compute.d.ts, add
tsconfig.json/tsconfig-transpile.json (matching code/made/wode/jode),
transpile switches from babel to `tsc -p tsconfig-transpile.json`.
- ComputedEntityMixin<C> is now generic over the compute payload (defaults
to esse's ComputeArgumentsSchema | undefined). Dependents (clusterFqdn,
clusterJid, timeLimit, computeQueue, errors) derive from C via indexed
access instead of being hardcoded `unknown`. `compute`'s own optionality
toggles on whether `undefined extends C` (`ComputeField<C>`): confirmed via
wode's Workflow.ts/Subworkflow.ts that TS2320 treats an optional property
(`compute?: X`, what esse's WorkflowSchema/SubworkflowMixinSchema actually
declare) and a required property typed `X | undefined` (what a naive
`compute: C` gives once C includes undefined) as *not* identical, so a
fixed modifier can never match both those schemas and esse's JobSchema
(compute required, no undefined) at once - only the conditional does.
- Fixed a latent runtime bug found while auditing: computedEntityMixin's
`compute` was getter-only. Applied after a generated schema mixin (as in
jode's Job.ts and wode's Subworkflow.ts), its getter-only descriptor
silently overwrote the schema mixin's get+set pair, so `job.compute = x`
threw `TypeError: Cannot set property compute which has only a getter`
even though the merged interface claimed compute was writable. `compute`
now has a real setter, verified with a descriptor probe before and after
plus a new regression test.
- bucket/filesRootDir guard against absent compute.cluster.fqdn (real per
the schema - both stay non-throwing, returning "").
- getApproximateCharge now throws a clear error when compute.timeLimit is
missing instead of silently producing a 0 charge (would otherwise let a
job through business_object.ts's balance check for free).
- Dropped @babel/* (7 packages) from dependencies - only moment/pluralize
are real runtime deps; added @mat3ra/tsconfig, typescript, ts-node,
@types/{node,mocha,pluralize}. Deleted .babelrc/.mocharc.json (ts-node
registers directly in the test script now, matching jode).
- Collapsed enums.ts's RMSNotificationsHandler class into a single
getNotificationValue() function: it was never exported, had exactly one
call site (always "PBS"), and the "adjust to make modular ... SLURM etc."
TODO it existed for was never implemented (no SLURM config object
anywhere) - a premature abstraction for a fixed 5-string computation.
EMAIL_NOTIFICATIONS' runtime output is unchanged.
Not run through eslint - same situation as jode: @exabyte-io/eslint-config
uses @babel/eslint-parser, which cannot parse the converted .ts files
(`npm run lint`/`lint:fix` now error standalone), and ide's own CI already
passes `skip-eslint: true` for this reason. The pre-commit hook itself is
unaffected: it runs lint-staged, whose src/js/**/*.js and tests/js/**/*.js
globs now simply match zero files and skip gracefully (verified). Wiring
up @typescript-eslint against the pinned eslint-config-airbnb chain is a
separate, larger change (same conclusion reached for jode).
Verified: `tsc --noEmit` clean, `npm test` 8/8 passing (incl. 2 new
regression tests for the setter fix and the timeLimit guard), `npm run
transpile` emits dist/js/*.d.ts including index.d.ts, and a runtime smoke
test against the compiled dist/js output (not just ts-node).
…ue object [release]
Was Array<{label, value}>, purely to support the array .find()-by-label
lookup that getNotificationValue() (née RMSNotificationsHandler) did
internally - now redundant now that lookup is a direct property access.
EMAIL_NOTIFICATIONS' runtime output is unchanged (verified).
The one real external consumer of the array shape is web-app's Cypress
e2e helper (ComputeFormWidget.ts, `.find(o => o.label === optionName)`) -
updated in a paired web-app change/commit to do a plain property lookup
instead, landed together with web-app's ide repin so nothing breaks
in between.
…ke our generated mixins [release] computedEntityMixin took `prototype: object` and returned `void`, with its own hand-rolled ComputedEntityHost<C> host-contract type (a `prop(name: string, default?): T` signature unrelated to the real one) - unlike every generated `*SchemaMixin` and hand-written mixin in `code`/`wode`/`jode`, which take `item: InMemoryEntity` and return `asserts item is T & Mixin`. - computedEntityMixin<C, S, T>(item: InMemoryEntity): asserts item is T & ComputedEntityMixin<C>. S is the extra host schema (owner/isExternal/ createdAt/workDir - not part of esse's bare job/workflow schemas, so any concrete schema missing them still satisfies S's constraint since they're all optional) layered on top of InMemoryEntity<S> itself, matching how a generated mixin's own `properties: InMemoryEntity<X> & X` + @ts-expect-error idiom works. - Dropped every `this.prop("compute.foo.bar", default)` dotted-path call. InMemoryEntity.prop's real signature is `prop<K extends keyof S>(name: K, ...)` - a single top-level key, not a dotted path - so passing "compute.cluster.jid" never actually typechecked against it (the previous ComputedEntityHost<C> silently allowed it via a fictional loose `prop(name: string)` signature that doesn't match the real one). Getters now read `this.compute?.cluster?.jid` etc. through the (now real) `compute` getter instead - verified identical at runtime against the compiled dist/js output, not just ts-node. - `set compute`/`setCompute` write directly to `this._json.compute` instead of calling `this.setProp("compute", value)`: setProp's signature (`value: S[typeof name]`) isn't call-site generic the way `prop`'s `<K extends keyof S>` is, so it resolves to the full S[keyof S] union rather than the one field's type when S is itself a still-abstract generic parameter (a real gap in code's own types, not something to work around with a cast here) - same fix and same reasoning as jode's Job.setMaterial. - owner/createdAt/workDir (bucket, filesRootDir, getApproximateCharge) now read via `this.prop("owner")` etc. instead of `this.owner`/`this.createdAt` /`this.workDir` direct property access - InMemoryEntity<S> only exposes S's keys through prop()/setProp(), not as direct instance properties, unless an actual mixin adds real getters for them. Verified: tsc clean, npm test 8/8, and a runtime check against the compiled dist/js output for every rewritten getter (clusterJid/clusterFqdn/ clusterFqdnShort/timeLimit/computeQueue/computePPN/computeNodes/errors/ bucket/filesRootDir) plus the compute setter and getApproximateCharge - all identical to pre-rewrite behavior. Also cross-checked by temporarily copying the rebuilt dist/js into wode's and jode's node_modules and running their own tsc+test suites against it (both green) before restoring their real installed copies via npm ci.
…d host fields [release] filesRootDir has zero consumers anywhere in the stack - web-app forked its own safer rewrite (Job.getFilesRootDirClient, taking owner as a param instead of reading this.owner, and using match?.[1] instead of a bare [1]) instead of using this getter, and nothing else calls it (verified via `grep -rn "\.filesRootDir\b"` across every reference/* package and web-app, plus ide's own tests). Dropped createdAt/workDir/owner.slug/owner.isPersonal/this.id from ComputedEntityHostSchema/the implementation along with it - they existed solely to support filesRootDir. owner.serviceLevel.nameBasedModifier stays (getApproximateCharge still reads it). Verified: tsc clean, npm test 8/8, and cross-checked by copying the rebuilt dist/js into wode's and jode's node_modules and running their own tsc+test suites against it (both green) before restoring their real installed copies via npm ci.
…eCharge [release]
getApproximateCharge read this.prop("owner")?.serviceLevel?.nameBasedModifier
directly - owner/serviceLevel with that shape are 100% web-app structures
(CoreAccount + ServiceLevel), not something esse's bare job/workflow schema
or ide itself has any business knowing about. The only real caller
(business_object.ts's ensureSufficientBalance) already loads the owner
account right there to check its balance, so it can compute the rate
modifier itself.
getApproximateCharge now takes `rateModifier` as part of its existing
`settings` object (defaults to 1, same as before) instead of reaching for
`this.prop("owner")`. owner/serviceLevel dropped from
ComputedEntityHostSchema entirely - nothing else in the mixin used them
(filesRootDir, the other consumer, was already removed).
web-app's own call site needs a paired follow-up (pass
`owner.serviceLevel?.nameBasedModifier` as `settings.rateModifier`) -
not done here, same as the ide.d.ts-shim/entityTypes.ts cleanup already
deferred earlier in this branch's history.
Verified: tsc clean, npm test 9/9 (added a regression test for
rateModifier), and cross-checked by copying the rebuilt dist/js into
wode's and jode's node_modules and running their own tsc+test suites
against it (both green) before restoring their real installed copies
via npm ci.
…n + infrastructureMixin [release] Audited every member against real usage across web-app, wode, jode, and the designer packages (wove/workflow-designer/job-designer/jove): compute/ setCompute/unsetCompute are genuinely shared (workflow-designer's own Subworkflow.tsx reads/writes subworkflow.compute directly), but clusterJid/clusterFqdn/clusterFqdnShort/timeLimit/computeQueue/computePPN/ computeNodes/computeNodesAndPPN/errors/getApproximateCharge/isExternalJob/ bucket/warnings/hasWarnings have zero non-Job callers anywhere in the stack - wode itself never touches any of them despite merging the type. - computedEntityMixin keeps only what's derived from `compute` itself: compute/setCompute/unsetCompute plus the compute-derived readouts (cluster*, timeLimit, computeQueue, computePPN, computeNodes, computeNodesAndPPN, errors). Makes sense for any entity carrying a compute config - Job, Workflow, or Subworkflow. - New infrastructureMixin (src/js/infrastructure.ts) holds what only makes sense for a job actually running somewhere: getApproximateCharge (billing), bucket/isExternalJob (cloud storage), warnings/hasWarnings. Requires computedEntityMixin already applied to the same host (reads clusterFqdn/ timeLimit/computeQueue/computePPN from it). - Dropped timePrediction entirely - hardcoded `return 0`, zero consumers anywhere, even in ide's own tests. - wode's Workflow.ts/Subworkflow.ts and jode's Job.ts need ZERO changes - verified by cross-checking their own tsc+test suites against the split dist/js before this commit. Neither ever referenced anything now in infrastructureMixin. Only web-app's own imports/jobs/job.ts (which calls job.bucket/job.getApproximateCharge/overrides warnings) needs infrastructureMixin added - separate follow-up commit there. Verified: tsc clean, npm test 9/9, a runtime check against the compiled dist/js output for both mixins together (clusterFqdn -> bucket -> getApproximateCharge -> isExternalJob -> warnings, all identical to pre-split behavior), and cross-checked wode's and jode's own tsc+test suites against the rebuilt dist before this commit.
… [release] Both were being silently shadowed in web-app's own imports/jobs/job.ts: computedEntityMixin(Job.prototype) runs after the class body, and Object.defineProperties always overwrites, so it was already clobbering job.ts's own get bucket()/get warnings() (verified empirically) - meaning job.warnings has ALWAYS returned [] in production and the hasBeenActiveMoreThanOnce message never actually surfaced. Rather than reorder application to make the mixin's own no-op warnings/isExternal-aware bucket win, dropping them here lets the host's own override take effect at all, and lets isExternal-based job bucketing be retired outright. - warnings/hasWarnings removed entirely - InfrastructureMixin now only has getApproximateCharge and bucket. web-app's own job.ts's warnings override (real behavior, no longer shadowed) is a separate follow-up commit there. - isExternalJob removed; bucket no longer branches on it - just clusterFqdn-derived, always. getExternalBucket/ExternalBucket (default.ts) dropped too - existed solely for the removed branch, zero consumers anywhere in the stack (verified). - wode/jode need zero changes (re-verified) - neither used warnings/ isExternalJob/bucket. Verified: tsc clean, npm test 9/9, runtime check against compiled dist/js confirming isExternalJob/warnings are gone and bucket still resolves correctly, plus wode's and jode's own tsc+test suites cross-checked against the rebuilt dist before this commit.
…n, not unconditional [release] Spotted by inspection: timeLimit/computeQueue were typed `Compute<C>["timeLimit"] | undefined` / `Compute<C>["queue"] | undefined` unconditionally, regardless of whether `compute` itself might be absent - even though esse's ComputeArgumentsSchema requires queue/nodes/ppn/ timeLimit *within* compute (required: ["queue","nodes","ppn","timeLimit"]). So for Job (compute required, no undefined in C), job.timeLimit was typed `string | undefined` even though it can only actually be undefined if compute itself is missing - which the type already promises won't happen. clusterJid/clusterFqdn didn't have this bug: `cluster` genuinely is optional *within* compute (not in that required list), so `Cluster<C>["jid"]`/`["fqdn"]` already carry `| undefined` correctly from the schema itself, no manual suffix needed. computePPN/computeNodes/errors don't have it either - their getters have `?? default` fallbacks (`this.compute?.ppn ?? 1`, `?? []`), so they're genuinely never undefined regardless of whether compute exists. - New `RequiredWithCompute<C, V>` - same `undefined extends C` conditional `ComputeField<C>` already uses, applied one level down: `V` when compute is guaranteed present, `V | undefined` when it might not be. - timeLimit/computeQueue now use it instead of a bare `| undefined` suffix. Verified: tsc clean, npm test 10/10 (added a compile-time regression test: required-compute's timeLimit assigns to plain `string` cleanly, optional-compute's needs @ts-expect-error against the same assignment), and cross-checked wode's and jode's own tsc+test suites against the rebuilt dist before this commit (Workflow/Subworkflow's timeLimit correctly stays `string | undefined`; Job's is now plain `string`).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…in generic [release]
ide was the last package in the stack still built with babel from plain JS, with its public types supplied by a hand-written, never-typechecked compute.d.ts. ComputedEntityMixin declared
readonly compute: unknown, which collides with every consumer's generated schema mixin also declaring a realcompute(TS2320 requires identical types across multiple extends) - forcing wode's Workflow.ts and jode's Job.ts to redeclarecomputewith a// TODO: fix ComputedEntityMixin and remove this, and wode's Subworkflow.ts toOmit<ComputedEntityMixin, "compute">with no explanation.tsc -p tsconfig-transpile.json.unknown.computewas getter-only. Applied after a generated schema mixin (as in jode's Job.ts and wode's Subworkflow.ts), its getter-only descriptor silently overwrote the schema mixin's get+set pair, sojob.compute = xthrewTypeError: Cannot set property compute which has only a gettereven though the merged interface claimed compute was writable.computenow has a real setter, verified with a descriptor probe before and after plus a new regression test.Not run through eslint - same situation as jode: @exabyte-io/eslint-config uses @babel/eslint-parser, which cannot parse the converted .ts files (
npm run lint/lint:fixnow error standalone), and ide's own CI already passesskip-eslint: truefor this reason. The pre-commit hook itself is unaffected: it runs lint-staged, whose src/js//*.js and tests/js//*.js globs now simply match zero files and skip gracefully (verified). Wiring up @typescript-eslint against the pinned eslint-config-airbnb chain is a separate, larger change (same conclusion reached for jode).Verified:
tsc --noEmitclean,npm test8/8 passing (incl. 2 new regression tests for the setter fix and the timeLimit guard),npm run transpileemits dist/js/*.d.ts including index.d.ts, and a runtime smoke test against the compiled dist/js output (not just ts-node).