Export CustomConverter Interface

[bitPogo]

Background

I have to use for a typescript project the CustomConverter option for asciidoctor.

Problem

I realised that the current types do not include a CustomConverter Interface. Interestedly extending form the Converter class break (at least for me) with:

TypeError: Cannot read property 'Converter' of undefined

      1 | import { Asciidoctor } from 'asciidoctor'
      2 | 
    > 3 | export default class MyCustomConverter extends Asciidoctor.Converter  {
        |                                                            ^
      6 | }

Solution

This could be easily solved by adding the following to the type definitions:

interface CustomConverter {
     convert(node: AbstractNode, transform?: string, opts?: any): string;
}

This could be implemented by the Converter class and a notable side effect the ConverterFactory could get rid of the any.

[ggrossetie]

Hey @bitPogo

Asciidoctor.js is not a standard JavaScript library. In fact, the code is transpiled from Ruby (which has a different class hierarchy model).
Your solution would work if Converter was a JavaScript class but it's not. Converter is actually a Ruby class transpiled to JavaScript.

As a result, your code won't work (or at least will be incorrect):

class MyCustomConverter extends Asciidoctor.Converter {
  // ...
}

If you want to create a custom converter, please take a look at: https://blog.yuzutech.fr/blog/custom-converter/index.html or https://asciidoctor-docs.netlify.app/asciidoctor.js/extend/converter/custom-converter/

[bitPogo]

Hey @Mogztter

thx for the reply.
I am aware of the fact, that this lib is in fact a ruby application. If you take a closer look, CustomConverter is actually an interface not a class. Meanwhile I wrote a prepare hook for node, which alters the type definition of asciidoctorjs on npm ci/i , in the way I intended. I will attach the content of that hook at the end of this post, so you take a look for yourself.
Thanks to the prepare hook I am now able to do that and I get the whole comfort of Typescript:

import AsciiDoc, { Asciidoctor } from 'asciidoctor'

class MyAwesomeConverter implements Asciidoctor.AbstractConverter {
...
}

But the hook is just a workaround never the less. So I am also happy with your approval to provide a PR. And by the way - thx for this lib! It makes my life actually much easier!

Attachment:

// This is a workaround for https://github.com/asciidoctor/asciidoctor.js/issues/1077
const fs = require('fs');
const ASCIIDOCTOR_DIR = `${__dirname}/../node_modules/asciidoctor/types`;

const file = fs.readFileSync(`${ASCIIDOCTOR_DIR}/index.d.ts`, 'utf8');
/* eslint-disable max-len */
const replacement = `interface AbstractConverter {
    /**
     * Converts an {AbstractNode} using the given transform.
     * This method must be implemented by a concrete converter class.
     *
     * @param node - The concrete instance of AbstractNode to convert.
     * @param [transform] - An optional String transform that hints at which transformation should be applied to this node.
     * If a transform is not given, the transform is often derived from the value of the {AbstractNode#getNodeName} property. (optional, default: undefined)
     * @param [opts]- An optional JSON of options hints about how to convert the node. (optional, default: undefined)
     *
     * @returns the {String} result.
     */
    convert(node: AbstractNode, transform?: string, opts?: any): string;
  }

  class Converter implements AbstractConverter {`;
/* eslint-enable max-len */
const replacement2 = 'register(converter: AbstractConverter, backends?: string[]): void;';
const fixed = file
  .replace('class Converter {', replacement)
  .replace('register(converter: any, backends?: string[]): void;', replacement2);

fs.writeFileSync(`${ASCIIDOCTOR_DIR}/index.d.ts`, fixed);

[ggrossetie]

Oh I see, so the idea is to get coding assistance from TypeScript when writing a customer converter, right?

I'm a bit torn because this interface does not really exist in the code but if we declare register(converter: AbstractConverter, backends?: string[]): void; then you will be "forced" to implement it.

It's worth noting that the underlying register function can take a class/function but also an instance. This is inherited from Ruby and arguably we should provide a cleaner JavaScript API but it's what it's.

Anyway, thanks for the detailed explanation, I now have a better understanding of the issue. I need to think it through to find a good balance between the flexibility of the API and the ease of development with TypeScript.

One good thing about TypeScript is that it guides you when discovering a new API, so I can definitely see the value of the AbstractConverter interface definition.

thx for this lib! It makes my life actually much easier!

Thanks! I'm glad to hear it 😳

[bitPogo]

it's worth noting that the underlying register function can take a class/function but also an instance. This is inherited from Ruby and arguably we should provide a cleaner JavaScript API but it's what it's.

Do you mean a factory or something like that? This could be also defined in way like:

type AbstractConverterFactory = () => AbstractConverter;
interface AbstractConverterConstructor {
  new(): AbstractConverter;
}

type RegisterPayload = AbstractConverterFactory | AbstractConverterConstructor | AbstractConverter;

class ConverterFactory {
...
     register(converter: RegisterPayload, backends?: string[]): void;
...
}

I'm a bit torn because this interface does not really exist in the code but if we declare register(converter: AbstractConverter, backends?: string[]): void; then you will be "forced" to implement it.

Yes, thats correct. I think devs even should be force to do that, since that is the only public that counts for the actual converter and the program will die without it anyways. Since that will not interfere with pure JS in any case, I see no major tradeoff here, beside the correct definition what register is really capable of. And you get rid of any, which is always a good thing, in my opinion.

[bitPogo]

A small amendment:
I stumbled over a thing for constructors, which I like to share in addition to my last post:

// node_modules/asciidoctor/types/index.d.ts
interface AbstractConstructor<T extends AbstractConverter> {
    new( ...params: never[] ): T;
    getInstance(): T;
 }
 
// consumer of the lib
interface MyAwesomeConstructor<T extends AbstractConverter> extends AbstractConstructor<T> {
    aAddMethod( param: 'hello' ): void;
}

class TestConverter implements AbstractConverter {
   private static staticHelloTypescriptFlag = '';

   public static  getInstance(): AbstractConverter {
         return new TestConverter( TestConverter.staticHelloTypescriptFlag )
    }

   public static aAddMethod( param: 'hello' ): void {
         TestConverter.staticHelloTypescriptFlag = param
         // do something more
         ...
   }
   
   constructor( a: string, ..._:any[] ) {
       // do something with a
       ...
   }    

   public convert( node: AbstractNode, transform?: string,  opts?: any): string {
         // interpret the actual AST
         ...
   }
}                                                  

function myBuilder(): AbstractConstructor<AbstractConverter> {
    const MyConverter: MyAwesomeConstructor<TestConverter> = TestConverter;
    MyConverter.aAddMethod('hello')
    ...
    return MyConverter;
}

I guess that should give all the flexibility you want, right?

[ggrossetie]

You can see two usages of the ConverterFactory.register function here: https://github.com/asciidoctor/asciidoctor.js/blob/master/CHANGELOG.adoc#v210-2020-01-26

Another usage would be to pass a "Ruby" class or instance transpiled by Opal.
But I don't think we should support this case in the type definition.
If the user is transpiling Ruby code to JavaScript, it's probably better to call directly the underlying function $register. In other words, the user won't use the Asciidoctor.js API.

If you can come up with a type definition that accepts both an ES6 class or instance please feel free to open a pull request.
You will probably need to update the tests:

asciidoctor.js/packages/core/types/tests.ts

Line 557 in 9e7e378

processor.ConverterFactory.register(new BlogConverter(), ['blog']);

asciidoctor.js/packages/core/types/tests.ts

Line 1085 in 9e7e378

processor.ConverterFactory.register(new BlankConverter(), ['blank']);

And you should add a new test to check that the ConverterFactory.register function can take a class as a parameter.

[bitPogo]

Thx for the reply!
I will open a PR as soon as I have time to do that. If I have any question, it's ok to ask them, right? (Sometimes owners are not ok to be disturbed, that's why I am asking).
Tow side questions (since I took a look in the code base):

1. backend and opts should also go into the definition of the constructor, right? I assume they are the same like in here. Also should the constructor be able to ignore those parameter?

2. Do you want also to allow some sort of factory methods? This should touch this line in a way like:

...
const functionalConverter = ( converterFactory,  backend, opts  ) => {
  let  converter;
  try {
    converter = new  converterFactory( backend, opts );
  } catch (err) {
    converter = converterFactory( backend, opts );
  }
  return converter;
}
...
const result = functionalConverter( converter, backend, opts) //asciidoctor-extensions-api.js#L1333

And gets typed liked the constructor. This could be done in a separate commit/PR

Side note:
Really I am so amazed by the way this repo is typed. I already tinkered with it...and it so awesome! It makes working with it so easy...really I so thankful, since I know, this was a huge pile of work.

[ggrossetie]

I will open a PR as soon as I have time to do that.

👍

If I have any question, it's ok to ask them, right? (Sometimes owners are not ok to be disturbed, that's why I am asking).

Feel free to ask 😉

backend and opts should also go into the definition of the constructor, right? I assume they are the same like in here. Also should the constructor be able to ignore those parameter?

Yes and they should be optional.

Do you want also to allow some sort of factory methods? This should touch this line in a way like:
And gets typed liked the constructor.

Not sure about that, I think the two options are sufficient for now.

This could be done in a separate commit/PR

Indeed, we can still re-evaluate once the issue is fixed.

Side note:
Really I am so amazed by the way this repo is typed. I already tinkered with it...and it so awesome! It makes working with it so easy...really I so thankful, since I know, this was a huge pile of work.

Thanks for noticing!
Truth be told, it took quite a bit of time 😅
At first I was hoping to fully automate the generation using https://github.com/englercj/tsd-jsdoc but it was not entirely possible so I had to refine the type definition by hand 😄