refactor(worker-javascript): extract Phase 3 helpers from Core.js#1039
Open
dqnykamp wants to merge 8 commits intoDoenet:mainfrom
Open
refactor(worker-javascript): extract Phase 3 helpers from Core.js#1039dqnykamp wants to merge 8 commits intoDoenet:mainfrom
dqnykamp wants to merge 8 commits intoDoenet:mainfrom
Conversation
Begin breaking up the 13,837-line Core class by lifting seven self- contained, low-coupling helpers into TypeScript modules. The pattern matches the existing composed siblings (Dependencies.js, ParameterStack): each module is constructed with a `core` back-reference, and Core retains a thin delegating wrapper for every method/property that was previously on the class so external callers (CoreWorker, tests, components, and `coreFunctions`-bound references) continue to work unchanged. Modules extracted: - DiagnosticsManager.ts — diagnostics queue + source-location walk - StateVariableNameResolver.ts — pure-function name resolution utilities - VisibilityTracker.ts — visibility state and save/suspend timers - StatePersistence.ts — save to localStorage / database - AutoSubmitManager.ts — debounced answer-submit queue - NavigationHandler.ts — handleNavigatingToComponent, navigateToTarget - ResolverAdapter.ts — adapter to the external Rust name resolver No behavior change. Core.js drops from 13,837 to 12,909 lines. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Continues the Core.js breakup with six moderate-effort modules. Same pattern as Phase 1 (composed sibling holding a `core` back-reference, thin delegating wrapper on Core for every public method/property). No behavior change. Modules extracted: - RendererInstructionBuilder.ts — owns componentsToRender, componentsWithChangedChildrenToRender, rendererState; the dast instruction stream sent to the renderer - ProcessQueue.ts — owns processQueue, processing, stopProcessingRequests; async request queue and entry-point scheduling (executeProcesses, requestAction, requestUpdate, requestRecordEvent) - ComponentLifecycle.ts — stateless: registration, ancestors, defining-child splicing, propagation to shadows - ChildMatcher.ts — child-group matching, adapter substitution, rendered-child filtering (recursion guard only) - DeletionEngine.ts — stateless two-phase component deletion - ActionTriggerScheduler.ts — owns stateVariableChangeTriggers, actionsChangedToActions, originsOfActionsChangedToActions; trigger polling and chained-action graph Core.js drops from 12,909 to 11,253 lines (this PR), 13,837 → 11,253 since the refactor began (~18.7%). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The deep guts of Core. Same composition pattern as Phases 1 and 2: each module is constructed with a `core` back-reference, and Core retains thin delegating wrappers for every public method/property. No behavior change. Modules extracted (in dependency order): - StateVariableDefinitionFactory.ts — synchronous shape-building: attribute / adapter / reference-shadow definitions, plus the shadow-conversion modifiers - StateVariableInitializer.ts — runtime initialization: lazy-resolving getters, dependency wiring, array-entry materialization, prop-index resolution - ComponentBuilder.ts — recursive component instantiation from serialized DAST plus the post-creation error-component flush - CompositeExpander.ts — composite expansion + replacement swap into active children + active/inactive marking; mutually recursive with ComponentBuilder via Core's delegators Subtle fix: `core.publicCaseInsensitiveAliasSubstitutions.bind(this)` calls inside CompositeExpander needed `bind(this.core)` — the wrapper on Core uses `this.componentInfoObjects`, so the bind target must be Core, not the manager. Core.js drops from 11,253 to 6,063 lines (this PR), 13,837 → 6,063 since the refactor began (~56.2%). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
…ndings
Two regressions from the Phase 3 extraction, surfaced in CI:
1. StateVariableInitializer.ts:425: \`let core = this;\` captured the
manager instead of Core, so the five \`core.addDiagnostic({...})\`
calls inside the inner array-callbacks (set up by
\`initializeArrayStateVariable\` for setArrayValue / getArrayValue /
etc.) failed at runtime with "core.addDiagnostic is not a function".
Fix: \`let core = this.core;\`. Caught by functionoperators.test.ts.
2. StateVariableDefinitionFactory.ts:1299: inside
\`stateDef.returnArrayDependenciesByKey = function () {...}\`, \`this\`
resolves to the stateDef at call time (not the manager), and
\`stateDef.arrayVarNameFromArrayKey\` is the method to call. The
blanket \`this.\` → \`this.core.\` transform in the extraction script
incorrectly added \`core.\` here. Fix: revert to
\`this.arrayVarNameFromArrayKey(key)\`. Caught by spreadsheet,
curve, curve.bezier, odesystem, and rectangle tests.
Audited the rest of the extracted modules for the same pattern
(\`function () {...}\` callbacks attached to stateVarObj/stateDef with
\`this.core.X\` references inside): no further occurrences.
ComponentBuilder.ts and CompositeExpander.ts have no inner-callback
patterns at all.
Verified: all 91 tests across the failing CI files plus 238 broader
regression tests pass.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
4 tasks
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
1 participant
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.
Note
Stacked on #1038 (which is stacked on #1036). This PR's diff includes Phases 1 and 2 until those land; review Phase 3 by reading the final commit or use the commits tab. Once #1036 and #1038 merge, this PR will auto-rebase and the diff will show only Phase 3.
Summary
Phase 3 of breaking up
packages/doenetml-worker-javascript/src/Core.js. The deep guts — four highly-interconnected modules that contain the algorithmic heart of Core. Same composition pattern as the prior phases (composed sibling holding acoreback-reference; thin delegating wrapper on Core for every public method/property). No behavior change.Net effect over all three phases:
Core.jsdrops from 13,837 → 6,063 lines (-56.2%).Modules extracted in Phase 3
StateVariableDefinitionFactory.tsStateVariableInitializer.tsComponentBuilder.tsCompositeExpander.tsSequencing rationale
Per the multi-phase plan, the four modules were extracted in dependency order:
StateVariableDefinitionFactoryfirst (pure shape-building, smallest blast radius), thenStateVariableInitializerwhich consumes its output, thenComponentBuilderandCompositeExpandertogether — they're mutually recursive (creating components triggers expansion; expanding creates components), and the cross-calls go through Core's delegators rather than trying to invert the call graph.Subtle fix
The diagnostics test suite caught one bug mid-phase:
core.publicCaseInsensitiveAliasSubstitutions.bind(this)insideCompositeExpanderwas binding the wrapper to the manager instead of to Core, sothis.componentInfoObjectswas undefined when later invoked. Fixed tobind(this.core). This is exactly the class of issue the plan called out as a risk (R4) — wrappers passed by reference need their bind targets explicitly aimed at Core.Test plan
🤖 Generated with Claude Code