-
-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add AbstractConvert & ConverterConstructor #1083
Conversation
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. |
Thanks @bitPogo I will take a closer look in the next few days.
You're right...
Not sure if this is the right way to do it but I'm using asciidoctor.js/packages/core/types/index.d.ts Lines 208 to 213 in 747caef
The main benefit is that you can still describe the type of some known/expected attributes.
I'm not familiar with the "unknown" type but it can be pretty useful to be able to redeclare the type. |
interface Callout extends Record<string, unknown> {
id?: string;
ordinal?: number;
} Regarding the docs - I simply c&p them. If it is really a Since there are some other Short story long...just tell me, what you like I should do and I change it. |
Amendment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
I left a nitpick but otherwise it's all good.
In a way, we are "breaking" the TypeScript API because now custom converters must implement So this change should land in 2.3 or even 3.0... thoughts? |
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 |
e4191a9
to
d42a6f8
Compare
Sry, I was a little bit busy last week (even for tabs), so it took a bit longer... |
d42a6f8
to
c6f16a7
Compare
There are two small issues in the API if you want to give it a try: issues with ":wave: help wanted" label. |
9b644db
to
b58fc82
Compare
I made a rebase...but something else broke...I interestingly I cannot reproduce the error locally... |
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:
You will need to run |
Confirmed, a bug was resolved on Asciidoctor core: asciidoctor/asciidoctor#3800 // 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. |
@bitPogo issue is fixed on master! I will rebase and merge tomorrow 😉 |
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.
b58fc82
to
8c2a2d4
Compare
All good, thanks again @bitPogo 👍 |
Awesome! I will check out some other issues next week and see how I can give there a hand. |
That would be great, thanks 🎉 |
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