🚧 Resolves #399, allow class and class instance to be registered as extensions

[ggrossetie]

I think it's fair to say that nobody will create a class using Opal syntax:

var superclass = Opal.const_get_qualified(Extensions, 'IncludeProcessor');
var scope = Opal.klass(Opal.Object, superclass, name, function () {});
Opal.defn(scope, '$initialize', function initialize () {
  Opal.send(this, Opal.find_super_dispatcher(this, 'initialize', initialize));
});
Opal.defn(scope, '$process', function (doc, reader, target, attrs) {
  reader.pushInclude(['included content'], target, target, 1, attrs)
}); 
return scope;

We could create a low level API to declare a class and define method on this class or we could pass a list of methods to register:

Low level API

var includeProcessorClass = asciidoctor.Class.create(Extensions, 'IncludeProcessor');
asciidoctor.Class.define(includeProcessorClass, 'process', function (doc, reader, target, attrs) {
  reader.pushInclude(['included content'], target, target, 1, attrs)
});

List of methods

Extensions.createIncludeProcessor = function (name, functions) {
  // ...
  for (var key in functions) {
    if (functions.hasOwnProperty(key)) {
      Opal.defn(scope, '$' + key, functions[key]);
    }
  }
});

@mojavelinux Once we agreed, I will implement the remaining functions 😉

[mojavelinux]

I think it's fair to say that nobody will create a class using Opal syntax:

I did. (https://gitlab.com/antora/antora/blob/master/packages/asciidoc-loader/lib/include/include-processor.js) But I get your point, it's not ideal.

[mojavelinux]

I like the API where you define a class name and a map of functions. This feels the most natural and makes it easy to add methods to the class that aren't part of the API. Perhaps if only a function is specified (not a map of functions), we can assume it's the process method, but that's up to you.

For the class name, we should support namespaces (e.g., Foo::Bar::BazExtension).

[mojavelinux]

We should try to rewrite the include processor in Antora to see how the model fits. For that use case, I needed to pass state to the initialize method, so that's something to consider.

[mojavelinux]

I like the API where you define a class name and a map of functions.

[ggrossetie]

I like the API where you define a class name and a map of functions.

Thanks for your feedback 😃

We should try to rewrite the include processor in Antora to see how the model fits. For that use case, I needed to pass state to the initialize method, so that's something to consider.

That's a good point, I will have a look.

[ggrossetie]

I like the API where you define a class name and a map of functions.

This API is working great for simple extension but if you want to add a state then you have to play with the scope.

I do have a solution but the instance is shared between functions:

// define a constant
const $callback = Symbol('callback');

let includeProcessor = asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', {
  setCallback: (callback) => {
    // Register the callback on the instance
    includeProcessorInstance[$callback] = callback;
  },
  process: (doc, reader, target, attrs) => {
    // Use the callback defined on the instance
    reader.pushInclude([includeProcessorInstance[$callback]('pass')], target, target, 1, attrs);
  }
});
// Instanciate an IncludeProcessor
let includeProcessorInstance = includeProcessor.$new();

includeProcessorInstance['$setCallback'](value => 'you should ' + value);
registry.includeProcessor(includeProcessorInstance);

As far as I know, it's not possible to override initialize function because the scope is lost and the function Opal.find_super_dispatcher will throw an exception.
@mojavelinux If you have a better idea, I'm all 👂

[mojavelinux]

As far as I know, it's not possible to override initialize function because the scope is lost and the function Opal.find_super_dispatcher will throw an exception.

I don't think overriding the initialize function is a hard requirement, especially since objects are open. We just need an opportunity to either hook into instantiation or pass state. We could even do both.

For the first approach, we could take a page from Java EE and reserve the postConstruct function. The custom class would override the initialize method internally and call this function at the end of the method (or however else we decide to do it).

For the second approach, we could allow an arbitrary object to be passed as the third argument. This would get stored in the instance variable "state" and could be accessed from the process method using this.state.

That would cover both requirements and still let us use the simple design of this API. wdyt?

[ggrossetie]

That would cover both requirements and still let us use the simple design of this API. wdyt?

Excellent ideas! 💯

[ggrossetie]

For the second approach, we could allow an arbitrary object to be passed as the third argument. This would get stored in the instance variable "state" and could be accessed from the process method using this.state.

this in the process method is not bound to include processor instance but to the "User Context" (global context ?).
I've tried to use the bind function but without success: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

Opal.defn(scope, '$' + key, functions[key].bind(scope));

In the following example, we can see that this is not bound to the include processor instance:

const $callback = Symbol('callback');
let includeProcessor = asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', {
  postConstruct: (context) => {
    this[$callback] = (value => 'you should ' + value);
  },
  process: (doc, reader, target, attrs) => {
    reader.pushInclude([this[$callback]('pass')], target, target, 1, attrs);
  }
});
let includeProcessorInstance = includeProcessor.$new();
this[$callback] = (value) => value; // ERROR: it will override the function defined in the postConstruct function
registry.includeProcessor(includeProcessorInstance);

EDIT: It's working when I define a function and I call this function directly:

let userFunction = functions[key];
Opal.defn(scope, '$' + key, function() {
  userFunction.bind(this).apply(this, arguments);
});

let includeProcessor = asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', {
  hello: function() {
    console.log('hello', this);
  }
});

let includeProcessorInstance = includeProcessor.$new();
includeProcessorInstance.$hello();

In the above example, this is bound to the include processor instance.

[s-leroux]

I read diagonally this thread. But one question @Mogztter what is the purpose of that statement?

userFunction.bind(this).apply(this, arguments);

As of myself, I would write either:

userFunction.bind(this)(arguments);
or
userFunction.apply(this, arguments);

Or did I missed something?

[ggrossetie]

I read diagonally this thread. But one question @Mogztter what is the purpose of that statement?

@s-leroux I'm trying to bind the extension context on this function but it's not working... I've tried bind then apply then, just to be sure, bind with apply 😀
I may have missed something or the issue is deeper (somewhere in the Opal's call stack).

If you want to give it a try, your help is more than welcome 🌈
The idea is to define a "state" and the challenge is to register this "state" on the include processor instance.

A simplified example:

let includeProcessor = asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', {
  postConstruct: (context) => {
    this['message'] = 'hello'; // register message property on the include processor instance
  },
  process: (doc, reader, target, attrs) => {
    reader.pushInclude([this['message']], target, target, 1, attrs); // this['message'] returns 'hello'
  }
});

[s-leroux]

Thanks for the reply @Mogztter
I must admit I was not very sure to understand what you were trying to achieve :D
But as I said, I didn't take the time to read the entire thread in depth.

When you said "context"--is this a JS context for JS consumption exclusively? Or do you expect a two way binding so that context would be accessible/modifiable from the Opal/Ruby side too?

Anyway, I could probably tackle into that. The only obstacle is I'm not familiar with the AsciiDoctor extensions nor "IncludeProcessor". If you had a simple self-contained Opal or Ruby usage example of custom processor, that could serve me as a starting point.

[s-leroux]

What about that:

  /**
    Create a new Opal class defining the $m and $self method for
    testing purposes.
  */
  const opalClass = function() {
    const C = Opal.Class.$new(Opal.Object /*, function(){} */);
    Opal.defn(C, '$m', function(){ return "original"; });
    Opal.defn(C, '$self', function(){ return this; });

    return C;
  };

/**
  Test case: override a method from an Opal class, with support to call the inherited/super
  implementation:
*/
    it("should allow calling the inherited method", function() {
      const base = opalClass();

      // ---------------------------------------------------------
      // This is the interesting part:
      // ---------------------------------------------------------
      const sub = outils.subclass(base, {
        '$m': function() {
          return "overridden+" + this.inherited.$m();
        },
      });
      // ---------------------------------------------------------

      const obj = sub.$new();
      assert.equal(obj.$m(), 'overridden+original');
    });

[ggrossetie]

Thanks for the reply @Mogztter
I must admit I was not very sure to understand what you were trying to achieve :D
But as I said, I didn't take the time to read the entire thread in depth.

We are just experimenting at this point 😛

When you said "context"--is this a JS context for JS consumption exclusively? Or do you expect a two way binding so that context would be accessible/modifiable from the Opal/Ruby side too?

It's a JS context or more precisely a JS variable available in the scope.

Anyway, I could probably tackle into that.

That would be great!

The only obstacle is I'm not familiar with the AsciiDoctor extensions nor "IncludeProcessor". If you had a simple self-contained Opal or Ruby usage example of custom processor, that could serve me as a starting point.

If I explain the code step by step maybe it will help you to understand.
The first step is to define a class that extend Extensions::IncludeProcessor:

var superclass = Opal.const_get_qualified(Extensions, 'IncludeProcessor');
var scope = Opal.klass(Opal.Object, superclass, name, function () {});

The second step, is to use Opal.defn to define/override functions on this class:

Opal.defn(scope, '$handles?', function () { return true; });

I can see that you are already familiar with Opal.defn: https://github.com/s-leroux/asciidoctor.js-pug/blob/7467602b6df8a4fa64fbb0a2d8ea17ebe1dce59f/lib/converter.js#L14

The only difference is that you are creating a new class that does not extend a superclass:

C = Opal.Class.$new(Opal.Object /*, function(){} */);

So that's what the code does.
Now we want to create an API where a user could define a list of functions:

asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', functions);

Each functions will be "register" to the class using Opal.defn.

In your code I can see that you are using an instance variable this.templates:

https://github.com/s-leroux/asciidoctor.js-pug/blob/7467602b6df8a4fa64fbb0a2d8ea17ebe1dce59f/lib/converter.js#L14-L16

This is exactly what we want to do.

asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', {
  postConstruct: () => {
    this.templates = templates;
  },
  handles: (name) => {
    return this.templates.has(name);
  }
});

[ggrossetie]

@s-leroux Thanks for you input. I think I found my mistake!
I was blindly using an arrow function (because I like the concise syntax) but arrow functions and functions are not the same!

An arrow function expression has a shorter syntax than a function expression and does not have its own this, arguments, super, or new.target. These function expressions are best suited for non-method functions, and they cannot be used as constructors.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

This is the reason why this was lost... 🤓
I will update my pull request.

[ggrossetie]

@mojavelinux @s-leroux What do you think about the method signature ?

[s-leroux]

Thanks for the help.

Actually, I think I've already written a somewhat similar but generic implementation to specialize Opal classes:

https://github.com/s-leroux/asciidoctor.js-pug/blob/opal-utils/lib/opal-utils.js

This module currently exports two methods:

• patch to patch just one method call in an existing class;
• subclass to create a new Opal derived class from JS (with support for calling the inherited method and context binding).

It should be easy to rewrite your asciidoctor.Extensions.createIncludeProcessor method to use that utility I think. Especially since overriding an Opal class from JavaScript is probably a feature we will need elsewhere.

Currently, I'm using the patch and subclass utility as part of implementing the common API in asciidoctor.js-pug as you suggested in s-leroux/asciidoctor.js-pug#1 (comment)

[ggrossetie]

This module currently exports two methods:
patch to patch just one method call in an existing class;
subclass to create a new Opal derived class from JS (with support for calling the inherited method and context binding).

👍

This is actually very close to the low level API described above:

Low level API

var includeProcessorClass = asciidoctor.Class.create(Extensions, 'IncludeProcessor');
asciidoctor.Class.define(includeProcessorClass, 'process', function (doc, reader, target, attrs) {
  reader.pushInclude(['included content'], target, target, 1, attrs)
});

subclass -> asciidoctor.Class.create
patch -> asciidoctor.Class.define

As suggested by @mojavelinux, we should support namespaces for the class name (e.g., Foo::Bar::BazExtension).

I think the API should also hide the $ prefix in the function names.

It should be easy to rewrite your asciidoctor.Extensions.createIncludeProcessor method to use that utility I think. Especially since overriding an Opal class from JavaScript is probably a feature we will need elsewhere.

Indeed!

Currently, I'm using the patch and subclass utility as part of implementing the common API in asciidoctor.js-pug as you suggested in s-leroux/asciidoctor.js-pug#1 (comment)

Excellent!
I think asciidoctor.js-pug/asciidoctor-template.js is the perfect playground to experiment with this low level API.
And when this API will be stable/mature enough, we could integrate it into Asciidoctor.js.

@mojavelinux What do you think ?

[mojavelinux]

I was blindly using an arrow function (because I like the concise syntax) but arrow functions and functions are not the same!

This trips up so many people, including myself until I finally hammered it into my brain. It's common because it's so unexpected. Once it bites you a few times, you never forget it.

[mojavelinux]

I really like the API @s-leroux has proposed. I also agree with @Mogztter that the leading $ in the method name should be optional (or perhaps even not allowed).

I still think, though, that we should continue to pursue the idea of a DSL-like API for defining an extension class. Underneath, it can still use the class extension utility @s-leroux created, but the extension author doesn't need to know that. Of course, if they want, they can still use the low-level class definition API directly.

[ggrossetie]

I still think, though, that we should continue to pursue the idea of a DSL-like API for defining an extension class.

👍
@mojavelinux Could you please review the createIncludeProcessor function. You should be able to rewrite the IncludeProcessor in Antora with this function. If it's OK I will create the other functions.

[s-leroux]

@mojavelinux, @Mogztter once again, thank you both for your support and for your time!
There are so many things in this thread. A couple of comments just off the top of my head below:

postContruct vs $initialize

asciidoctor.Extensions.createIncludeProcessor('StaticIncludeProcessor', {
  postConstruct: () => {
    this.templates = templates;
  },
  handles: (name) => {
    return this.templates.has(name);
  }
});

What is the purpose exactly of the postContruct method? In my own implementation I used the $initialize method for the purpose of setting the object's instance variables (this.template = ...). I followed that path because from what I understood the $initialize method was the Opal/Ruby-way of defining a constructor. Or is there some semantic difference I missed with postConstruct?

Concerning Extensions.createIncludeProcessor() I see it here: https://github.com/Mogztter/asciidoctor.js/blob/79e5ffb6bebaa52c223643b66253ca75dc0acac2/src/asciidoctor-extensions-api.js#L298

Indeed this is close to what I written myself. An issue though is you lose the original method. So I can't call the base implementation from a specialized one. For example, I would like to be able to write something like that:

let MySpecializedProcessor = asciidoctor.Extensions.createIncludeProcessor('MyBaseProcessor', {
  initialize: () => {
    this.inherited.initialize(); // call base constructor
    doThisAndThat();
  }

  process: (doc, reader, target, attrs) => {
    if (thisOrThat)
      doSomething();
    else
      this.inherited.process(doc,reader,target,attrs); // or this.super.process(...)
  }
});

API

I still think, though, that we should continue to pursue the idea of a DSL-like API for defining an extension class. Underneath, it can still use the class extension utility @s-leroux created, but the extension author doesn't need to know that. Of course, if they want, they can still use the low-level class definition API directly.

I would push toward a solution based on a low-level API and then build a more user-friendly DLS-like API on top of that for the most common use cases. Such separation would probably help in testing and maintenance, and, as you said, by providing the low-level API, users with special needs would still have some options before having to dig into Opal.

The case of the $-methods

I think the API should also hide the $ prefix in the function names.
I also agree with @Mogztter that the leading $ in the method name should be optional (or perhaps even not allowed).

I've ignored that until now because I wasn't sure how/why those '$' are appearing before the method names. At some point, I considered adding "behind the scene" the '$' to the user-provided methods:

let C = subclass(base, {
  process: function() { ... },
});

In the C class above, the user-provided process method would have silently been converted to $process, eventually overriding the base implemetation of that method. I've seen two issues with that solution though:

• what if the user want to call the base implementation in an inheritance chain? In that case, (s)he has to be aware of that pitfall to call the base method with the $ prefix:

let C = subclass(base, {
  process: function() { if (thisOrThat) this.inherited.$process(); },
});

• by blindly adding a $ before all user-supplied methods, isn't there a risk we override by mistake a base method while this wasn't the intend of the user?

let C = subclass(base, {
  name: function() { return thisOrThat; }, // OUPS: will override $name by mistake
});

I would suggest the low-level API to not try doing anything special for the $. It's low-level after all. The user has to be aware some method names are starting by $. In addition, not trying to be clever here will guarantee non-$ identifiers won't clash with $-identifiers by mistake.

But the high-level API could provide a wrapper for the few methods expected to be overriden from JS. So developers using only the high-level API for well defined use cases will never see a $-method.

We could achieve that by forwarding the $-method calls to their non-$ counterparts:

In pseudo-code:

Extensions.createIncludeProcessor = function (name, functions) {
  // call the low-level API to create a derived class
  C = createNewDerivedClass(name);
  
  // wrap $-methods expected to overriden by the user into plain JavaScript functions:

  // do that for each method
  // (based on the assumption addMethodForClass will return the base implementation)
  const $original_process = addMethodForClass(C, "$process", function(...) { this.process(....) }); // the $-method forwards to the non-$ method
  addMethodForClass(C, "process", $original_process); // the non-$ method receive the original implementation (or undefined, btw)


 // override methods with user provided function
 for([name, fn] in functions) addMethodForClass(C, name, fn);
}

If I'm not too wrong with that kind of wrapper, calling $convert (either directly or through Opal-generated code) would fallback to calling "convert" (no $) with the original implementation by default. But the user can override that by providing its own "convert" (no $) implementation.

Or did I missed something?

[mojavelinux]

I wasn't sure how/why those '$' are appearing before the method names.
I would suggest the low-level API to not try doing anything special for the $. It's low-level after all.

The '$' prefix is just Opal's generic way of not stepping on existing symbols. It means "Ruby method". But in this context, everything is a Ruby method (since we are defining a Ruby class), so we don't need to leak this detail. It's an abstraction that is leaking out of Opal that is just making things ugly and providing no other benefit.

[mojavelinux]

what I understood the $initialize method was the Opal/Ruby-way of defining a constructor.

It's the name of the class constructor method in Ruby. (minus the leading $ of course)

What is the purpose exactly of the postContruct method? In my own implementation I used the $initialize method for the purpose of setting the object's instance variables (this.template = ...).

That's precisely what the postConstruct method is for. But now that I think about it a bit more, we already have control over the definition of the class, so we might as well just use the name "initialize" and not try to be fancy. So let's just call it "initialize". Internally, this will need to be responsible for either invoking the constructor or running just after it (if there's some technical limitation).

[mojavelinux]

thank you both for your support and for your time!

Indeed! Community FTW!

[mojavelinux]

what if the user want to call the base implementation in an inheritance chain?

I believe it's possible to bind a super somehow, perhaps using getSuper() if all else fails.

[s-leroux]

But now that I think about it a bit more, we already have control over the definition of the class, so we might as well just use the name "initialize" and not try to be fancy. So let's just call it "initialize". Internally, this will need to be responsible for either invoking the constructor or running just after it (if there's some technical limitation).

Actually, once we can call the super/inherited methods, we can call the base $initialize method too. So there no need to be fancy or invent a new semantic here: just call the inherited initialize method to invoke the base constructor.

I believe it's possible to bind a super somehow, perhaps using getSuper() if all else fails.

OK. Here is a possible implementation (I used inherited as super is a keyword in JS--if this is a problem we could find some way to overcome this limitation maybe).

@Mogztter @mojavelinux I encourage you to take a look at the code since this works surprisingly well to override methods and constructors from JS while still allowing the specialized code to call the inherited method using the syntax this.inherited(base, '$m'):

https://github.com/s-leroux/asciidoctor.js-pug/blob/3bb9e08adcc11540cfe69e8e0dfff09ba191ebb6/test/opal-utils.js#L214

    it("should allow calling the inherited method", function() {
      const base = opalClass();
      const sub = outils.subclass(base, {
        '$m': function() {
          return "overridden+" + this.inherited(base, '$m')();
        },
      });

      const obj = sub.$new();
      assert.equal(obj.$m(), "overridden+original");
    });

    it("should allow calling the inherited constructor", function() {
      const base = opalClass();
      const sub = outils.subclass(base, {
        '$initialize': function() {
          this.derivedInitializeCalled = true;
          this.inherited(base, '$initialize')();
        },
      });

      const obj = sub.$new();
      assert.isTrue(obj.baseInitializeCalled);
      assert.isTrue(obj.derivedInitializeCalled);
    });

    it("should allow calling the inherited method [several levels of subclassing]", function() {
      const base = opalClass();
      const sub = outils.subclass(base, {
        '$m': function() {
          return "overridden+" + this.inherited(base, '$m')();
        },
      });
      const sub2 = outils.subclass(sub, {
        '$m': function() {
          return "level3+" + this.inherited(sub, '$m')();
        },
      });

      const obj = sub2.$new();
      assert.equal(obj.$m(), "level3+overridden+original");
    });

(subclass implementation is here)

[mojavelinux]

just call the inherited initialize method to invoke the base constructor.

I think @Mogztter was suggesting there is a technical reason why we cannot, though I'm not sure I understand what that limitation is since I was able to do it with some hackery in Antora.

[mojavelinux]

OK. Here is a possible implementation (I used inherited as super is a keyword in JS--if this is a problem we could find some way to overcome this limitation maybe).

Seems completely fine to me 👍

[ggrossetie]

Lot of ideas, I love it!

💡 super/inherited

Opal is using the following code to call super:

Opal.send(this, Opal.find_super_dispatcher(this, 'initialize', initialize));

@s-leroux Is there a reason why you choose to use base.$$proto[name].bind(this); in the inherited function instead of Opal.find_super_dispatcher(this, name, func) ?
However I do agree that we should provide an API to call the super function because Opal.send(this, Opal.find_super_dispatcher(this, 'initialize', initialize)); is not straightforward 😉

So 👍 for inherited

💡 postContruct vs $initialize

It was a workaround because this was lost, and also because the user will need to call super.
In a high-level API, I think it's better to define:

postConstruct: () => {
  this.templates = templates;
}

than:

initialize: () => {
  Opal.send(this, Opal.find_super_dispatcher(this, 'initialize', initialize)); // user: "What is this boilerplate... I don't even understand this code"
  this.templates = templates;
}

or even:

initialize: () => {
  this.inherited.initialize();
  this.templates = templates;
}

We could call Opal.send(this, Opal.find_super_dispatcher(this, 'initialize', initialize)); for the user when initialize is overridden but in my opinion its error prone because the behavior is not consistent.

postConstruct makes it clear that we are running after super and the behavior is consistent: if you override a "base" method you have to manually call super.
But again, we could consider that initialize is not a "base" method and/or that it's clear enough that super is going to be called...

So I'm leaning toward postConstruct for the high-level API.

💡 $-methods

The '$' prefix is just Opal's generic way of not stepping on existing symbols. It means "Ruby method". But in this context, everything is a Ruby method (since we are defining a Ruby class), so we don't need to leak this detail. It's an abstraction that is leaking out of Opal that is just making things ugly and providing no other benefit.

I 100% agree.
The high-level API should not use $.

📔 Summary

• Define an inherited function
  ❓ should we define this function in the low-level API or in the high-level API
  ❓ should we use Opal.find_super_dispatcher or $$proto[name]
• Use postConstruct to define a function that needs to be executed after the class has been instantiated
• Do not use $ in the high-level API

@mojavelinux @s-leroux Do you agree ?

[s-leroux]

super/inherited

Is there a reason why you choose to use base.$$proto[name].bind(this); in the inherited function instead of Opal.find_super_dispatcher(this, name, func) ?

Of course: I wasn't aware of find_super_dispatcher. After a quick look at the sources, it seems better. I don't have the time today, but I will try to use that now.

postContruct vs $initialize

A more fair comparaison would be between:

postConstruct: () => {
  this.templates = templates;
}

versus (don't pay attention to the $ for now: this is an orthogonal issue)

$initialize: () => {
  this.templates = templates;
  this.inherited(base, '$initialize')()
}

Besides that, I have several issues with the postConstruct solution. In decreasing order of importance to me:

Calling the base constructor with a modified parameter list

There is one use case you can't address with postConstruct: what if the derived constructor wants to change the parameter passed to the base constructor. For example, to provide default values. Or to modify a parameter:

$initialize: (x) => {
  x = somehowModify(x);
  const y = inferSomeDefaultValue();

  this.inherited(base, '$initialize')(x, y);
}

I think the closest Ruby equivalent would be calling super(...) with an explicit argument list (correct me if I'm wrong here).

New paradigm

postConstruct will introduce yet another new
semantic for chaining constructors. And also, this would create a
different semantic to call the base constructor (implied) whereas base methods would still have to be explicitly called.

JavaScript developers already know they should explicitly call the prototype/super constructor. From what I saw, Ruby developers know they should explicitly call super. So why doing something fancy here?

Failing early

Another corner case is you can't fail early after sanity check:

postConstruct: () => {
  if (cantWorkProperly)
    throw SomeError();
}

vs

$initialize: () => {
  if (cantWorkProperly)
    throw SomeError();

  this.inherited(base, '$initialize')()
}

With the postConstruct implementation, you have to go through the whole inherited constructor chain before failing.

$-methods

I think we agree. No $ in the high level API.

The high-level API has knowledge of the intended use case. So it is in position to hide some implementation details or awkward syntax.

The low level API should be a rebust base foundation so you can build the high-level API without bothering with too many Opal-related stuff. On the other hand, the low-level API should be as transparent as possible by not making too many assumptions on what the developers want to do.

[s-leroux]

You've done a lot of great work @Mogztter. And you have a deep knowledge of Opal and Asciidoctor. On my side, I may have more ideas about how to bind JavaScript in doing what we need.

So, I suggest we work on a client-provider relationship:
Tell me what are your functional needs. Then I implement that the closest I can from your expected interface and you see if you can build the high-level API on top of that. And we iterate the process.

For example, is such interface suitable for you?

createSubclass('SomeBaseClassName', {
  $initialize: () => {
    this.templates = templates;
  },
  $doThis: (name) => {
    return this.templates.has(name);
  }
});

Could you refactor createIncludeProcessor based on that? Do you want a different function name/signature? Do you have enough control on your side to do the high-level magic, like providing non-$ wrappers to the commonly overridden methods? Or do you need something more?

Ideally, the low-level API could be developed as part of a different project/{sub,}repository, so we will be forced to stick with the defined interface without being tempted to tweak the other side of the mirror. On my side, I could use the same low-level API for asciidoctor.js-pug so we can see if the API is generic enough to be used in a different context.
@Mogztter @mojavelinux What do you think of that?

[ggrossetie]

postContruct vs $initialize

Besides that, I have several issues with the postConstruct solution. In decreasing order of importance to me:

1. Calling the base constructor with a modified parameter list
2. New paradigm
3. Failing early

In my opinion, postConstruct and initialize are not mutually exclusive. In 1 and 3, I would argue that what you really want is to override the initialize the method to do "advanced" initialization.

postConstruct is indeed a new "paradigm" to address a simple need: "As a user, I want to pass a state to the initialize method".
Keep in mind that we want a DSL-like API and postConstruct is just a shorthand for (I believe) the most common use case.
It's also worth noting that initialize and this.inherited.initialize() are uncommon for a JavaScript developer.

But maybe I'm biased because I'm from the Java EE world and postConstruct is a familiar keyword... so I already know this paradigm 😉

So, as you mentioned, the real question is: Is it worth it to introduce this new paradigm ?

Since there's a debate, we could use the YAGNI principle and leave it for now.

@mojavelinux @oncletom @hsablonniere Do you have an opinion ?

Could you refactor createIncludeProcessor based on that? Do you want a different function name/signature? Do you have enough control on your side to do the high-level magic, like providing non-$ wrappers to the commonly overridden methods? Or do you need something more?

I think we should move forward with this issue and see how things are evolving. For now I don't think we should use a low-level API because we are still experimenting and the API is subject to changes.

Ideally, the low-level API could be developed as part of a different project/{sub,}repository, so we will be forced to stick with the defined interface without being tempted to tweak the other side of the mirror.

As long as the low-level API is generic enough and don't do magic, I think we are fine 😃

[mojavelinux]

In following this discussion, it seems clear to me that postConstruct and initialize are not mutually exclusive. They have different goals and are thus both important. What @Mogztter is trying to do is provide a lifecycle hook for converters that are only enhancing an existing converter. This is a very different use case from creating a brand new converter from scratch, which will likely need fine-grained control over the initialize method (and its call to super). The postConstruct is only used if present, so you can have both, one or the other, or neither. And that seems like a pretty good deal to me.

I agree strongly with @s-leroux that we want the APIs to feel as natural as possible to a JavaScript developer. And we're fortunate to have your input on this design, @s-leroux.

And open question in my mind is where to put the API for creating Ruby classes. Ideally, it would go in Opal itself, though I don't necessarily want to impose our needs on that project. So perhaps what we're looking at is something like opal-class-builder or something that we can than depend on in Asciidoctor.js. It doesn't make sense to be for it to live in Asciidoctor.js or a downstream from it. It really belongs upstream from Asciidoctor.js.

The high-level API belongs in Asciidoctor.js as it's providing a shorthand for making extension classes.

[ggrossetie]

And open question in my mind is where to put the API for creating Ruby classes. Ideally, it would go in Opal itself, though I don't necessarily want to impose our needs on that project.

We could ask on Gitter: https://gitter.im/opal/opal 😃
Maybe someone in the Opal community is already working on something or had already thought about it.

[mojavelinux]

We could definitely ask. And I'm not saying that Opal won't be open to the idea. But it feels to me like we would be imposing because this is a higher-level thing than what Opal needs to be concerned about. Opal is not really trying to provide a porcelain API. It's just dealing with the transpilation. Perhaps this could end up being a subproject of Opal. That would certainly make sense.

[s-leroux]

@Mogztter @mojavelinux Thank you for your replies.

Indeed, I have much lower-level needs than the scope of Extensions.createIncludeProcessor. Namely derivating arbitrary Opal classes and eventually creating entire hierarchies of Opal classes from JavaScript. I use that both on asciidoctor.js-pug as well as in much larger extend in some internal project (basically an Asciidoctor -> MongoDB document bridge).

Since I need such low-level interface and I have already written and tested it (on Node.js) I was pushing for that to be included "somewhere" upstream instead of creating a new derived project just for that. But, I've somewhat missed the point the goal of createIncludeProcessor-like methods was to enhance an existing converter. Not creating one from scratch. So yes, why not seing that with the Opal team?
_{...but I wanted so much working with you Dan & Guillaume ;)}

[ggrossetie]

So yes, why not seing that with the Opal team?

I pinged Elia and as expected they are very open minded about this idea 😛

Also please keep us posted on the API you're going toward, to be informed, avoid gotchas, etc. maybe some of the stuff you'll come up with can become part of the runtime.js api

@s-leroux You can definitely share your ideas with them!

...but I wanted so much working with you Dan & Guillaume ;)

But we are working together ❤️

[mojavelinux]

But we are working together ❤️

Exactement !

[ggrossetie]

@mojavelinux What do you think about these functions ? The only purpose is to hide $new()... do you have a better idea ? should we remove these functions for now ?

[ggrossetie]

I want to merge this changes before the first release of Asciidoctor.js 1.5.7

[thom4parisot]

I arrive a bit late in the game, sorry 😅

Is there any reason not to use native ECMAScript classes/objects ?
I can imagine something like:

const {BaseConverter} = require('asciidoctor.js/converter');
const Asciidoctor = require('asciidoctor.js')();

class TextConverter extends BaseConverter {
  constructor(backend, options) {
    super(doc, options);
    this.something = options.foo || 100;
  }

  convert(node, transform, args) {
    // ...
  }
}

Asciidoctor.Converter.Factory.$register(new TextConverter('txt'));

[ggrossetie]

In theory we could wrap Opal classes into ECMAScript classes but I'm pretty sure it's not possible to extend Opal classes. The reason is that Opal uses "special" classes because the object model in Ruby is largely different.

But it gave me an idea, createIncludeProcessor could return an ECMAScript class. So instead of calling $new() we could instantiate the class with the new operator. Then the API could detect that we are using an ECMAscript class and "unwrap" the class before calling Asciidoctor internals.

I need to check if it's possible 🤓