Change node access to mathWithTransform to a map interface

[jhugman]

As a followup to #2166 (review) this wraps mathWithTransform in a ObjectWrappingMap.

This explicitly does not attempt to get into the lazy factory/initialization code, which sounds like smarter minds than I are looking at (e.g. #2076 and #1975).

From my run of tests/benchmark/index.js this doesn't significantly impact performance.

[josdejong]

This is really nice 😎 . Less need for customs.js throughout the code base again.

Good point about the lazy functionality that is currently in mathWithTransform. This was quite a struggle to get right in the past, but I think thanks to the Map interface we're now using more and more this becomes quite straightforward. I think what we need is a LazyMap class, and then we can create mathWithTransform as a LazyMap.

[josdejong]

Just for clarity: the idea of the "lazy" functions is: the one time creation of the functions takes quite some time, so let's only execute that creation the first the the function is actually invoked. Ideally, when requesting all keys of the map, we get back all the keys, no matter whether they are initialized already or not.

[jhugman]

That _importFactory looks pretty intense!

How I feel, when I'm working with bootstrapping code.

Why don't we land this, then we can think about how best to tackle it.

I agree, a LazyMap was the way I was thinking about it too, but I'm not quite ready to agree with you that switching this to a Map will make this "quite straightforward".

[josdejong]

😂 No problem at all!

Let me give this a try to see if this idea of LazyMap indeed works. If so, I'll extend your PR and have this step in one go. If it's more complicated let's merge your PR as-is and refactor mathWithTransform itself later.

[josdejong]

It was indeed a tough one to replace mathWithTransform with a LazyMap 😅 . Had to make changes in the code that automatically generates entry files for example. I have something working now, see #2279.

I realized this PR (#2264) will introduce a breaking change, so we'll need to publish it as such.

There is a feature allowing you to compile/parse/evaluate arguments yourself in a custom, "raw" function, see docs: https://mathjs.org/docs/expressions/customization.html#custom-argument-parsing. This function signature was:

rawFunction(args: Node[], math: Object, scope: Object)

But now math is changed into a Map:

rawFunction(args: Node[], math: Map, scope: Object)

I think though that we've already broken this API: right now scope is a Map too (since mathjs 9.4.0). Example https://github.com/josdejong/mathjs/blob/develop/examples/advanced/custom_argument_parsing.js doesn't work anymore because of this. I simply didn't think about it, and unfortunately there are no unit tests guarding this (so we should create tests for this).

Let's think through a bit if we can (still) make this backward compatible. And let's think through if we we want to make more related breaking changes (if we're going to break the API anyway let's utilize the opportunity).

[jhugman]

There is a feature allowing you to compile/parse/evaluate arguments yourself in a custom, "raw" function

Gotcha, we did see an issue and a suggested documentation fix.

Example https://github.com/josdejong/mathjs/blob/develop/examples/advanced/custom_argument_parsing.js doesn't work anymore because of this. I simply didn't think about it, and unfortunately there are no unit tests guarding this (so we should create tests for this).

I wonder if adding a node-test which exercises everything in the examples directory and fails if there's anything in std::err or a non-zero exit code would be a good place to start.

[jhugman]

I'm going to close this, due to lost context. Please re-open if you need it.

[josdejong]

👍 thanks for your inputs anyway! We'll look into it again when the need arises.