✨ Add AbstractConvert & ConverterConstructor

[bitPogo]

Please note: this changes opts of the convert method as well. It's now
'unknown', so consumer are able to redeclare the type for their need.

closes #1077

[mojavelinux]

Is unknown the way you declare a generic key-value pair object in TypeScript? Because that's what it should be.

The API docs are technically wrong, because JSON is a string. That's different than a JavaScript object which is what is accepted.

[ggrossetie]

Thanks @bitPogo I will take a closer look in the next few days.

The API docs are technically wrong, because JSON is a string. That's different than a JavaScript object which is what is accepted.

You're right...
I don't know why but in my mind JSON was equivalent to "JavaScript Object" and "JSON string" was a JavaScript Object as a string.
Anyway, we should fix this issue in another pull request to replace globally the term JSON where it should be JavaScript Object.

Is unknown the way you declare a generic key-value pair object in TypeScript? Because that's what it should be.

Not sure if this is the right way to do it but I'm using interface to describe a key-value pair object:

asciidoctor.js/packages/core/types/index.d.ts

Lines 208 to 213 in 747caef

	interface Callout {
	[key: string]: any;
	id?: string;
	ordinal?: number;
	}

The main benefit is that you can still describe the type of some known/expected attributes.

It's now 'unknown', so consumer are able to redeclare the type for their need.

I'm not familiar with the "unknown" type but it can be pretty useful to be able to redeclare the type.

[bitPogo]

Is unknown the way you declare a generic key-value pair object in TypeScript? Because that's what it should be.

unknown is the better any - so a consumer has to define what it is and cannot make false assumptions (and if they want to use any, they can override it). In difference to any you even have to cast or redefine it. See here. But in this case it does not even bother since we do not use it. The redefinition must happen when you use effectively the parameter - so the consumer side. This person should known how that thing what is in use looks like.
However if you mean Record<string, unknown>, I can change it to that. I used also unknown, since I really not known, which type is intended. So when it should be just a simple HashMap/Dictionary (I have no clue how Ruby call it), then is Record<string, unknown> really the way to go.

Not sure if this is the right way to do it but I'm using interface to describe a key-value pair object:

Callout are probably similar:

interface Callout extends Record<string, unknown> {
  id?: string;
  ordinal?: number;
}

Regarding the docs - I simply c&p them. If it is really a object (aka HashMap), I can change that, too.

Since there are some other any's floating around, maybe this could be the start to get rid of them. So this would also clear one of the points in the ToDo of the README.adoc.

Short story long...just tell me, what you like I should do and I change it.

[bitPogo]

Amendment:
Maybe (I do not known where), the repo should have a example to redeclare the type and make it testable. Actually I like how Vue does it, but this would need a separate tsconfig, therefore in my head in a separate PR.

[ggrossetie]

Looks good to me 👍
I left a nitpick but otherwise it's all good.

[ggrossetie]

In a way, we are "breaking" the TypeScript API because now custom converters must implement Asciidoctor.AbstractConverter.

So this change should land in 2.3 or even 3.0... thoughts?

[ggrossetie]

Since the master branch contains a major upgrade of Opal (from 0.11 to 1.0) it will be part of Asciidoctor.js 3.0

[bitPogo]

Sry, I was a little bit busy last week (even for tabs), so it took a bit longer...
The browser tests are currently broken, this is the reason, why this patch is currently red (which this patch does not tinker with).
However, if there is anything I can help with, just tell me...It's such a lovely project, so I would be glad to be of service beyond this patch.

[ggrossetie]

@bitPogo No worries! I fixed the failure in #1089 could you please rebase on master one last time?

[ggrossetie]

However, if there is anything I can help with, just tell me...It's such a lovely project, so I would be glad to be of service beyond this patch.

There are two small issues in the API if you want to give it a try: issues with ":wave: help wanted" label.
Let me know if you are interested and don't hesitate to ask for help/clarification 🤗

[bitPogo]

I made a rebase...but something else broke...I interestingly I cannot reproduce the error locally...

[ggrossetie]

I think it's a "regression", or a false assertion on our side. @mojavelinux is merging quite a few pull requests on Asciidoctor core.

For reference, the error is:

  1) Node.js
       Node.js
         Loading
           should get the line of number of a block when sourcemap is enabled:
     AssertionError: expected 3 to be undefined
      at Context.<anonymous> (spec\share\asciidoctor-spec.js:359:49)
      at processImmediate (internal/timers.js:461:21)

I interestingly I cannot reproduce the error locally...

You will need to run npm run clean --prefix packages/core and then npm run build:quick --prefix packages/core to build Asciidoctor.js against the latest version of Asciidoctor core (master).

[ggrossetie]

Confirmed, a bug was resolved on Asciidoctor core: asciidoctor/asciidoctor#3800
Now the line number is available on a preamble, so we need to update the assertion:

// preamble
expect(blocks[0].getLineNumber()).to.equal(3)
expect(blocks[0].getBlocks().length).to.equal(1)
expect(blocks[0].getBlocks()[0].getLineNumber()).to.equal(3)

I will fix this issue.

[ggrossetie]

@bitPogo issue is fixed on master! I will rebase and merge tomorrow 😉

[ggrossetie]

All good, thanks again @bitPogo 👍

[bitPogo]

Awesome! I will check out some other issues next week and see how I can give there a hand.

[ggrossetie]

That would be great, thanks 🎉