MathJax-src icon indicating copy to clipboard operation
MathJax-src copied to clipboard

Better types

Open dpvc opened this issue 2 years ago • 2 comments

This PR improves the type checking of the MmlNodes and Wrappers by adding more generics to reduce the use of any types. This allows the removal of some as MmlNode and other casting of types, but means there are more generics to specify for many classes.

To make all this work, the mix-ins needed to be updated. This involves adding interfaces for both the mix-in instance and classes that weren't used before (or were only used in a rudimentary way). The complicated part was that the mix-in base class is not allowed to take generics from the class that is extending it, so the mechanism for handling the mix-in had to be adjusted to work around that. This is done by having the SVG and CHTML classes us a function that take generics and that function creates a class that extends the mix-in using he generics from the function (not from the extended class).

For example, the CHTML wrapper for the MmlMi is created via:

/**
 * The ChtmlMi wrapper class for the MmlMi class
 */
export const ChtmlMi = (function <N, T, D>(): ChtmlMiClass<N, T, D> {

  const Base = CommonMiMixin<
      N, T, D,
      CHTML<N, T, D>, ChtmlWrapper<N, T, D>, ChtmlWrapperFactory<N, T, D>, ChtmlWrapperClass<N, T, D>,
      ChtmlCharOptions, ChtmlVariantData, ChtmlDelimiterData, ChtmlFontData, ChtmlFontDataClass,
      ChtmlMiClass<N, T, D>
    >(ChtmlWrapper);

  return class ChtmlMi extends Base implements ChtmlMiNTD<N, T, D> {

    /**
     * @override
     */
    public static kind = MmlMi.prototype.kind;

  };

})<any, any, any>();

Here, we use function <N, T, D>() to make a function that depends on three generics (the DOM Node, Text, and Document classes), and then get the base mix-in class using those generics. Then we create the ChtmlMi class from that base and return it. We use the generics <any, any, any> for <N, T, D>, but within the definition of ChtmlMi, the generic types are honored; that is, the checking that something that is supposed to be N is really N and not T, and that these are as expected in the use of N, T, and D within the mix-in base class. This typechecking was not being done before.

In addition, we change the ts/output/common/OutputJax.js to just ts/output/common.js for consistency with ts/output/chtml.js and ts/output/svg.js, and use a common dom property rather than the jax-specific chtml and element properties, again for consistency among the output jax.

Finally, the names of the classes and interfaces are put into camel-case for better consistency of naming.

This looks like a big update, and it is, but many of the changes follow a common pattern (adding the interfaces and associated generics, and updating the mix-in usage), are to remove casting that is no longer needed, or just change the names of things (camel-case or move to dom name). This is mostly just about getting the types right, and the code itself is changed very little other than that.

dpvc avatar Jun 09 '22 15:06 dpvc

PS: I also think it is worthwhile keeping your explanations for the PR around somewhere. Either in the code or better yet in our documentation somewhere.

zorkow avatar Aug 05 '22 10:08 zorkow

I think they need some additional commenting. I made some comments where I believe some extra comments should go.

OK, I've made the changes your requested. See if the new comments are what you were looking for.

I also think it is worthwhile keeping your explanations for the PR around somewhere

OK, I think it can be in the documentation somewhere. We need a lot more of that, I'm afraid.

dpvc avatar Aug 05 '22 14:08 dpvc