fix: bump math-expressions so odeSystem works in the browser - #1667
Merged
Conversation
… browser Any document containing an <odeSystem> rendered as a red "numeric is not defined" banner instead of a document, and any graph of a solution drew no curve. `dopri`, the solver, comes from numeric.js, bundled inside math-expressions. numeric builds most of its helpers at load time with the `Function` constructor, and the generated bodies reference a bare `numeric` — e.g. `if(typeof _s === "undefined") _s = numeric.dim(x);`. Functions made that way are evaluated in global scope, so that reference resolves only if `numeric` is a property of the global object. numeric registered itself there solely through Node's `global`, which neither a browser main thread nor a web worker has, so every generated helper threw on first call — and `dopri` reaches them immediately. Both sides of the worker boundary went through that path: the worker evaluating `$$f(1)`, and the main-thread renderer sampling the same solution to draw the curve. math-expressions 2.0.0-alpha95 registers numeric itself, which covers both. No source change was needed, so the regression test is a Cypress spec rather than a Vitest one: Vitest runs under Node, where `global` exists and numeric registers itself, so the suite stayed green through the whole bug. Only a real browser reproduces it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The curve assertion took the longest `d` string among every path in the board and required it to exceed 5000 characters. Measured, the curve is 23418 characters and the x-axis tick path is 4830 — a 3% margin, and the axis path grows with the tick count, so a different viewport could clear the floor with no curve drawn at all. Only the axes and their ticks are stroked in the axis color, so excluding that color leaves the curve alone; the assertion now counts its segments, which separates a sampled curve from anything else by an order of magnitude. Also drop the changeset's closing paragraph on why the regression test is Cypress rather than Vitest: that is repo-internal test strategy, not user-visible behavior, and it is recorded in the spec's own comment and the PR description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRxvB5p6GSwm2jGD1pMBvu
The comment restated the whole bug narrative that the changeset and PR description already carry. Keep what a reader of this spec needs — why `numeric` has to be global, why the test cannot be a Vitest one, and why both sides of the worker boundary are asserted — and drop the rest. Also drop an `async` on the `cy.window()` callback that never awaited anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRxvB5p6GSwm2jGD1pMBvu
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.
Any document containing an
<odeSystem>renders as a red "numeric is not defined" banner instead of a document, and any graph of a solution draws no curve. This bumpsmath-expressionsto2.0.0-alpha95, which fixes it, and adds a Cypress regression test.The bug
dopri, the ODE solver, comes from numeric.js, bundled inside math-expressions. numeric builds most of its helpers at load time with theFunctionconstructor, and the generated bodies reference a barenumeric:Functions made with
Function(...)are evaluated in global scope, so that reference resolves only ifnumericis a property of the global object. numeric registered itself there through a Node-ism:Neither a browser main thread nor a web worker has
global, so the assignment was skipped and every generated helper threw the first time it was called — anddoprireaches them immediately.Both sides of the worker boundary go through that path, and both failed:
ODESystem'snumericalSolutions(packages/doenetml-worker-javascript/src/components/dynamicalSystems/ODESystem.js), which is what killed the whole document;createFunctionFromDefinition→returnODESolutionFunctioninpackages/utils/src/components/function.ts.On the main thread the error reads
dim is not a functionrather thannumeric is not defined, because the virtual keyboard has a button withid="numeric"and the named-element global makes the slot look occupied while still being useless to the generated code.window.global = window.global || windowinpackages/doenetml/src/doenetml.tsx(the react-mathquill hack) does not help: it runs in the module body, after every import — math-expressions included — has already been evaluated.The fix
Upstream, in Doenet/math-expressions#86:
lib/mathjs.jsregisters numeric itself, which covers every runtime. Released as2.0.0-alpha95. No source change was needed here — this is the version bump (the rootpackage.jsonandpackages/utils/package.json, which pinnedalpha93) plus a test.The regression test
packages/test-cypress/cypress/e2e/dynamicalsystem/odesystem.cy.js(@group2) loads an<odeSystem>with both a$$f(1)evaluation and a graphed solution, then asserts the rendered value and that a curve was actually plotted — one assertion per side of the worker boundary.It is a Cypress spec rather than a Vitest one for a specific reason worth recording: Vitest cannot catch this class of bug. It runs under Node, where
globalexists and numeric registers itself, sopackages/doenetml-worker-javascript/src/test/dynamicalsystem/odesystem.test.tspassed cleanly through the entire lifetime of the bug. Only a real browser reproduces it.Verified in both directions with a clean rebuild each way (
npm run clean:cachebetween them — wireit does not treatnode_modulesas a build input, so a dependency bump alone will not invalidate its cache):2.0.0-alpha94Expected to find element: #pVal, but never found it— the document is the error banner2.0.0-alpha95The failure screenshot on alpha94 shows the "numeric is not defined" banner in place of the document.
Other checks
cypress/e2e/dynamicalsystem/*— 3 passing (the new spec plus bothcobwebpolylinetests).npm run test -w @doenet/doenetml-worker-javascript -- run src/test/dynamicalsystem— 16 passing, 5 files.518.99), the solution curve plots, andglobalThis.numeric.dimis a function on the main thread.🤖 Generated with Claude Code