Update MathJS to v12.4.3 and handle the new approach to scope/partitioned map

[pjanik]

https://www.pivotaltracker.com/story/show/187950451

This fix doesn't look pretty, but it seems to work. @kswenson, you're right that the problematic change on the MathJS side was introduced here: josdejong/mathjs#3150.

[cypress]

3 flaky tests on run #3835 ↗︎

0

201

28

0

3

Details:

Merge pull request #1371 from concord-consortium/187950451-mathjs-update
Project: codap-v3	Commit: aa877f665d
Status: Passed	Duration: 10:12 💡
Started: Jul 25, 2024 10:32 AM	Ended: Jul 25, 2024 10:42 AM

  table.spec.ts • 1 flaky test

View Output

Test		Artifacts
case table ui > table cell editing > edits cells		Test Replay Screenshots

  slider.spec.ts • 2 flaky tests

View Output

Test		Artifacts
Slider UI > updates variable name		Test Replay Screenshots
Slider UI > adds new variable value		Test Replay Screenshots

Review all test suite changes for PR #1371 ↗︎

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.03%. Comparing base (b308ce7) to head (30cd1c4).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1371      +/-   ##
==========================================
- Coverage   85.84%   83.03%   -2.82%     
==========================================
  Files         521      521              
  Lines       25266    25281      +15     
  Branches     6875     6831      -44     
==========================================
- Hits        21690    20992     -698     
- Misses       3301     3983     +682     
- Partials      275      306      +31     

Flag	Coverage Δ
cypress	67.22% <96.87%> (-5.72%)	⬇️
jest	53.51% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kswenson]

👍 Thx for doing this. Quick review for now -- I'll do a more thorough review on the next round. Two suggestions before that, however:

1. There's a failing jest test that appears related that should be fixed. (I suspect the failing cypress test is just one of our usual flaky ones.)
2. A comment that explains how this interacts with MathJS would be very helpful. I'm interested in the question of whether we're relying on documented aspects of MathJS, in which case the change that triggered this should have been semver-major, or perhaps more likely, we're relying on undocumented implementation details, in which case we should at least document that fact so that we'll know where to start looking the next time this comes up and you're not around. 😉

[kswenson]

Also, since PartitionedMap has no meaning in CODAP, perhaps we can call it something like scopeMap when used as an argument that might give some indication of its purpose.

[pjanik]

@kswenson, sorry, I actually forgot to push the last changes, that's why the test was broken. The last commit fixes that and reactors a few things a bit.

I've actually added a type called MathJSPartitionedMap. Not sure, but I'd actually suggest to keep it, as that's what might allow us to orient ourselves in the MathJS code. If it's just scope, it can be confusing. While paritionedMap doesn't say much in CODAP, it's something passed from MathJS, and at least it explains why we need to access .a property.

A comment that explains how this interacts with MathJS would be very helpful. I'm interested in the question of whether we're relying on documented aspects of MathJS

I based that implementation on the fact that the official documentation described scope as an object that implements the Map interface, and also on this example (which seems to be outdated now, btw):
https://mathjs.org/examples/advanced/custom_scope_objects.js.html
But apparently, MathJS isn't that strict about that, and the the example is now outdated and nonfunctional.

My last commit adds a comment about all that where the MathJSPartitionedMap is defined.

I'm also talking directly to MathJS maintainer:
josdejong/mathjs#3150
He's very responsive. Apparently, they might be considering a better solution for our use case. It would be good if someone else also follows that thread in the PR in case I'm working on another project when the fix arrives. I'll get notified anyway, but just in case.

[pjanik]

@kswenson, small update. I think this long conversation between the MathJS maintainer and me can actually answer some of your questions whether we're using documented or undocumented features (there's no clear answer, but I think we're fine at this point): josdejong/mathjs#3150

I'm going to modify this PR to implement getRootScope as suggested here:
josdejong/mathjs#3150 (comment)
This seems to be safe enough approach. At least the main maintainer is now aware of the issues. 😉
I also asked if they could export getRootScope helper themselves. That would be even safer, and it'd be easy to update our code.

[pjanik]

Okay, I've just pushed the final commit. I followed the suggested approach:
josdejong/mathjs#3150 (comment)
I could make checks more strict, but this would make testing more annoying, so I hope it's a reasonable compromise.

I hope that new types and variable names are also clearer.

[kswenson]

👍 Looks great! Thx for the fix and for contacting the maintainer to get some additional guidance.