Unable to use new types contained within Units in some operations

[nickewing]

I have been attempting to implement a new type BigFraction from fraction.js/bigfraction.js. So far things have gone pretty well, except I've run into an issue with Units. Several of the definitions of operations are self referential for the Unit, Unit signature, such as addScalar below:

export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ typed }) => {

  const addScalar = typed(name, {
    'number, number':  // ..,
    'Complex, Complex': // ..,
    'BigNumber, BigNumber': // ..,
    'Fraction, Fraction': // ..,

    'Unit, Unit': function (x, y) {
      // ...

      const res = x.clone()
      res.value = addScalar(res.value, y.value) // <-- self reference
      res.fixPrefix = false
      return res
    }
  })

  return addScalar
})

This factory defines an immutable list of signatures for the function addScalar and thus when the Unit, Unit signature calls the typed function, the only available signatures are the ones defined within this factory. It will not have access to any signatures merged into addScalar by other factories later on.

E.g. when the following new factory is imported and the typed function is merged, the original Unit, Unit signature of addScalar does not have access to the new signatures.

export const createAddScalarBigFraction = factory("addScalar", dependencies, ({ typed }) => {
  return typed("addScalar", {
    "BigFraction, BigFraction": // ...,
    "BigFraction, BigNumber": // ...,
    "BigNumber, BigFraction": // ...
  })
})

I have found as a work around that I can add an entry to typed.conversions, and BigFractions will be converted to BigNumbers, but this is not ideal as it requires that all unit math be done using BigNumbers when BigFraction may be preferable.

Is there any existing work around for this?

[josdejong]

Thanks for bringing this up Nick. Very well explained. I've had troubles with this myself too, but not yet looked into this in more detail.

I do not know of a solution, but it is essential in order to make it possible to really extend all functions with new signatures on the fly. Any ideas to solve this self-referencing issue would be very welcome! I will keep it in the back of my mind.

[nickewing]

It seems that the factories in their current state need to be able to resolve the most recent definition of the typed-function that they're defining. I tried a couple options that might work, the first of which would be to add another special case resolveSelf into the dependency injection like so:

diff --git a/src/core/function/import.js b/src/core/function/import.js
index a3ed133f0..0420a393a 100644
--- a/src/core/function/import.js
+++ b/src/core/function/import.js
@@ -262,6 +262,8 @@ export function importFactory (typed, load, math, importedFactories) {
             dependencies.math = math
           } else if (dependency === 'mathWithTransform') {
             dependencies.mathWithTransform = math.expression.mathWithTransform
+          } else if (dependency === 'resolveSelf') {
+            dependencies.resolveSelf = () => math[name]
           } else if (dependency === 'classes') { // special case for json reviver
             dependencies.classes = math
           } else {
@@ -269,7 +271,11 @@ export function importFactory (typed, load, math, importedFactories) {
           }
         })

       if (instance && typeof instance.transform === 'function') {
         throw new Error('Transforms cannot be attached to factory functions. ' +
diff --git a/src/function/arithmetic/addScalar.js b/src/function/arithmetic/addScalar.js
index c91e6d220..510abcd4f 100644
--- a/src/function/arithmetic/addScalar.js
+++ b/src/function/arithmetic/addScalar.js
@@ -2,9 +2,9 @@ import { factory } from '../../utils/factory'
 import { addNumber } from '../../plain/number'

 const name = 'addScalar'
-const dependencies = ['typed']
+const dependencies = ['typed', 'resolveSelf']

-export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ typed }) => {
+export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ typed, resolveSelf }) => {
   /**
    * Add two scalar values, `x + y`.
    * This function is meant for internal use: it is used by the public function
@@ -17,7 +17,7 @@ export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ ty
    * @return {number | BigNumber | Fraction | Complex | Unit}     Sum of `x` and `y`
    * @private
    */
-  const addScalar = typed(name, {
+  return typed(name, {

     'number, number': addNumber,

@@ -39,11 +39,9 @@ export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ ty
       if (!x.equalBase(y)) throw new Error('Units do not match')

       const res = x.clone()
-      res.value = addScalar(res.value, y.value)
+      res.value = resolveSelf()(res.value, y.value)
       res.fixPrefix = false
       return res
     }
   })
-
-  return addScalar
 })

Alternatively, a method could be passed as a separate argument to the factory outside of DI like this:

diff --git a/src/core/function/import.js b/src/core/function/import.js
index a3ed133f0..0420a393a 100644
--- a/src/core/function/import.js
+++ b/src/core/function/import.js
@@ -269,7 +271,11 @@ export function importFactory (typed, load, math, importedFactories) {
           }
         })

-      const instance = /* #__PURE__ */ factory(dependencies)
+      const applySelf = function () {
+        return math[name].apply(null, arguments)
+      }
+
+      const instance = /* #__PURE__ */ factory(dependencies, applySelf)

       if (instance && typeof instance.transform === 'function') {
         throw new Error('Transforms cannot be attached to factory functions. ' +
diff --git a/src/function/arithmetic/addScalar.js b/src/function/arithmetic/addScalar.js
index c91e6d220..533e05c40 100644
--- a/src/function/arithmetic/addScalar.js
+++ b/src/function/arithmetic/addScalar.js
@@ -4,7 +4,7 @@ import { addNumber } from '../../plain/number'
 const name = 'addScalar'
 const dependencies = ['typed']

-export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ typed }) => {
+export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ typed }, applySelf) => {
   /**
    * Add two scalar values, `x + y`.
    * This function is meant for internal use: it is used by the public function
@@ -17,7 +17,7 @@ export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ ty
    * @return {number | BigNumber | Fraction | Complex | Unit}     Sum of `x` and `y`
    * @private
    */
-  const addScalar = typed(name, {
+  return typed(name, {

     'number, number': addNumber,

@@ -39,11 +39,9 @@ export const createAddScalar = /* #__PURE__ */ factory(name, dependencies, ({ ty
       if (!x.equalBase(y)) throw new Error('Units do not match')

       const res = x.clone()
-      res.value = addScalar(res.value, y.value)
+      res.value = applySelf(res.value, y.value)
       res.fixPrefix = false
       return res
     }
   })
-
-  return addScalar
 })

In this setup, the use of applySelf instead of resolveSelf looks a little cleaner, but only makes sense to typed-functions. That may be all that is needed though given that I think that is the only type of factory definition that can be extended by imports. Additional information about the typed function, such as the signatures, would not be available to the current factory in this setup however.

Any thoughts on these approaches?

[josdejong]

That is a very smart solution Nick, thanks!

I think your two ideas are very close, and I like applySelf the most too. I have the feeling though that we should implement this self reference solution purely in typed-function, and not mix it with the dependency injection and the internal math namespace. Else the solution works only partly I'm afraid.

Maybe we can expose a self reference of the typed-function on each of the original functions, or pass the self reference as extra argument. Something along those lines:

var myFunction = typed({
  'MyType, MyType': function (a, b) {
    return new MyType(this.applySelf(a.value, b.value))
  }
});

or:

var myFunction = typed({
  'MyType, MyType': function (a, b, self) {
    return new MyType(self(a.value, b.value))
  }
});

[josdejong]

And a totally different possibility: we could see if we can make the need for self-references redundant. Like we already have two versions of add (add and addScalar), we could create a separate function as soon as the need arises for self reference. I'm afraid this will result in an explosion of functions (which are hard to name) but I'm not sure, maybe it can work out.

[nickewing]

It may be possible to split up all the modules so that no self references are necessary, but there are about 100 function modules that currently have self references, so that approach might be fairly unwieldy.

I made a PR in typed-function that implements the this based approach you suggested. I agree that it is a better fit in that project.

I first tried your other suggestion of passing a self function into each function, since I thought the resulting definitions looked pretty nice. It worked fine in all of the tests in that project, but when using it within math.js, there were around 70 test failures. For example, definitions like the one found in the matrix module become an issue because two signatures share the same function and take a variable number of arguments.

[josdejong]

Fixed via #1903

[josdejong]

Published now in v7.1.0 🎉