Closed pjanik closed 1 month ago
![]() |
![]() |
![]() |
![]() |
![]() |
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 |
View
Output
Test | Artifacts | |
---|---|---|
case table ui > table cell editing > edits cells |
Test Replay
Screenshots
|
View
Output
Test | Artifacts | |
---|---|---|
Slider UI > updates variable name |
Test Replay
Screenshots
|
|
Slider UI > adds new variable value |
Test Replay
Screenshots
|
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 83.03%. Comparing base (
b308ce7
) to head (30cd1c4
). Report is 9 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
@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: https://github.com/josdejong/mathjs/pull/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.
@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): https://github.com/josdejong/mathjs/pull/3150
I'm going to modify this PR to implement getRootScope
as suggested here:
https://github.com/josdejong/mathjs/pull/3150#issuecomment-2248101774
This seems to be safe enough approach. At least the main maintainer is now aware of the issues. :wink:
I also asked if they could export getRootScope
helper themselves. That would be even safer, and it'd be easy to update our code.
Okay, I've just pushed the final commit. I followed the suggested approach: https://github.com/josdejong/mathjs/pull/3150#issuecomment-2248101774 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.
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: https://github.com/josdejong/mathjs/pull/3150.