Some api feedbacks while defining typescript types for remark and unified modules

[Rokt33r]

As I mentioned in #383 (comment), I found defining typescript types for remark is quite confusing. In my experience, it is some kind of sign that we can improve our api more straight forward. So I want to share some of my idea. But I don't mind if you don't accept it because most of them should cause huge breaking changes.

Manipulating constructor form outside

To be honest, I'm not a fan of prototype based programming. Although it makes our expressions fluent, it usually makes them chaotic. One example is copying a constructor function via https://github.com/wooorm/unherit. In our code, we are using it to prepare Parser and Compiler constructors. And to edit it, we have to access prototype. It just makes the problem worse.

Suggestion

To deal with this problem, I think we should split the roles with classes.

1. class Parser keeps configuration for parse method and is instantiated when the processor become frozen.
2. class ParseContext keeps current state while parsing and is instantiated when start parsing.

Tokenizer

Overloading

Overloading is sometimes useful. But it definitely makes code hard to understand. For an example, to make custom tokenizer in typescript, we have to do like the below.

function tokenizeMention(
  eat: remarkParse.Eat,
  value: string,
  silent: true
): boolean | void
function tokenizeMention(eat: remarkParse.Eat, value: string): Unist.Node | void
function tokenizeMention(
  eat: remarkParse.Eat,
  value: string,
  silent?: boolean
): Unist.Node | boolean | void {

https://github.com/mike-north/remark/pull/1/files#diff-807c8f2d3bf10faeecc6dd0779edcb29R24

Functions with props

Tokenizer is a function with some props. This makes code looks monkey patched.

Suggestion

We can make Tokenizer as class and separate tokenize method into two.

// InlineTokenizer is abstract class and extended from Tokenizer
class MentionTokenizer extends InlineTokenizer {
  notInLink: boolean = true

  // abstract method from InlineTokenizer. It forces users to implement `locate` method
  locate(value: string, fromIndex: number): number

  // abstract methods from Tokenizer.
  tokenizeSilently(eat: Eat, value: string)
  tokenize(eat: Eat, value: string)
}

Too many options to configure remark

Same to tokenizer, Processor#use method also uses overloading a lot. So the typing was quite difficult and made our code meaninglessly bloated.

Implementation:

https://github.com/unifiedjs/unified/blob/master/types/index.d.ts#L20-L32
https://github.com/unifiedjs/unified/blob/master/types/index.d.ts#L147-L161

Test:

https://github.com/unifiedjs/unified/blob/master/types/unified-tests.ts#L31-L72

Suggestion

I think we should provide one of the ways below without global configuration.

unified
  .use(parse(parseOptions))
  .use(stringify(stringifyOptions))
  // With array
  .use([parse(parseOptions), stringify(stringifyOptions)])

// Or 
unified
  .use(parse, parseOptions)
  .use(stringify, stringifyOptions)
  // With array of tuple(plugin, options)
  .use([[parse, parseOptions], [stringify, stringifyOptions]])

[fazouane-marouane]

I'll take a look at those Issues/PRs tonight. I'd like to see if I can give a hand!

[fazouane-marouane]

I see that all the overriding is ill suited indeed and how we can easily end up in a lot of unnecessary boilerplate. That been said, I don’t know how a class would help, since it will most likely mean duplication of code, of some sorts.

I wonder if an adaptor function that transforms any (eat: Eat, value: string, silent?: boolean)=> Node | void into a tokenizer type would cover most cases 🤔

[Rokt33r]

I don’t know how a class would help.

Could you tell me which class wouldn't be helpful?

I wonder if an adaptor function that transforms any (eat: Eat, value: string, silent?: boolean)=> Node | void into a tokenizer type would cover most cases 🤔

It also looks good to me. :)

[fazouane-marouane]

Hi again, (there's been some family issues that kept me from online presence 😅)

So, the way I see it is that there is little we can do to avoid overloading, unless we split implementation of tokenize and tokenizeSilently, like you suggested. It's only a feeling, but I guess that may be more cumbersome to do.

As to the adaptor/coerce trick, it is based on the following: 1/ having a strict type definition of tokenize (and other method with the use of overloading) and then 2/ implement the tokenizers in a more "forgiving" (as in "not so strict") fashion like so:

// import Node type
declare namespace Unist {
  class Node {
    type = "node";
  }
}
// import remark-parse
declare module remarkParse {
  type Eat = (value: string) => {};

  interface TokenizerCall {
    (e: Eat, value: string, silent: true): boolean;
    (e: Eat, value: string, silent?: false): Unist.Node;
    (e: Eat, value: string, silent?: boolean): Unist.Node | boolean;
  }

  // Totally equivalent to TokenizerCall
  // type TokenizerCallAlt = ((e: Eat, value: string, silent: true) => boolean) &
  //  ((e: Eat, value: string, silent?: false) => Unist.Node) &
  //  ((e: Eat, value: string, silent?: boolean) => Unist.Node | boolean);
}

// define a generic magic coerce function that will transform any collection of function overloads into a simple (and forgiving) function definition
function unistCoerce<TFunc extends (...args: any[]) => any>(
  func: (...args: Parameters<TFunc>) => ReturnType<TFunc>
): TFunc {
  return func as any;
}

// use it to define your custom tokenizers
const tokenizeMention = unistCoerce<remarkParse.TokenizerCall>(
  (
    eat: remarkParse.Eat,
    value: string,
    silent?: boolean
  ): Unist.Node | boolean => {
    return new Unist.Node();
  }
);

I'd be very interested in seeing how this may help your codebase in the real world 😊

[wooorm]

Sorry for letting this sit.

Manipulating constructor from outside

Yes, that’s confusing. But I don’t know of a different way to do it. How could a plugin configure a constructor that is not yet instantiated?
If there is a better way to do this, we should solve it in micromark.

Tokenizer: Overloading

That does make it a bit harder to understand. But also much more performant. Same reason locators exist. If this can be fixed, should be done in micromark as well.

Tokenizer: Functions with props

This seems to be solved in micromark: https://github.com/micromark/micromark/blob/master/lib/atx-heading.ts ✨

Too many options to configure remark

This is an issue with unified. And has to do with how other related middleware-accepting projects work, and how unified is used from the CLI. Additionally, as you mentioned, global settings add complexity. And presets. I don’t think this can be made easier.

[Rokt33r]

I think we can ignore Tokenizer: Overloading and Too many options to configure remark. I think I was typescript nazi when I opened this issue... 😀

Other two answers look fine to me. I'll keep watching changes in micromark. 👍

So I think this issue can be closed now. Thanks for the answers!

[wooorm]

hah! thanks for posting the questions, it’s good that the API is reviewed and for it to be easy and a to have a relatively small surface area.

How the API of micromark looks, is still up for debate, so if you have thoughts on that, I’d appreciate them there: micromark/micromark#5