Use the Asciidoctor.js API (for consistency)

[ggrossetie]

I think it's important to use the Asciidoctor.js API for consistency. So I've removed the $node object and I'm using the following API:

• node.getContent() // String
• node.getImageUri() // String
• node.getAttributes() // Object
• node.getRoles() // Array

@mojavelinux Do you think that node.getRoles() should return an Set instead of an Array ?

[mojavelinux]

Do you think that node.getRoles() should return an Set instead of an Array ?

That is one of the items on the roadmap for 2.0 (options as well). In the case of roles, it would have to be an ordered set as order is significant when translated to CSS.

[ggrossetie]

I think JavaScript Set are ordered because elements returned when iterating are in the insertion order:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/entries

That is one of the items on the roadmap for 2.0 (options as well).

Ok so we could get ahead Asciidoctor core and return a Set ? Does it sound reasonable ?

[mojavelinux]

Yes, you can move in that direction as that will be the API core will eventually provide / support.

[hsablonniere]

Any reason why you want/need it to b a set? I would love the node to be easily jsonifiable and an array would help a lot.

[s-leroux]

Do you think that node.getRoles() should return an Set instead of an Array ?

That is one of the items on the roadmap for 2.0 (options as well). In the case of roles, it would have to be an ordered set as order is significant when translated to CSS.

@Mogztter @mojavelinux Speaking of 2.x roadmap, can't we consider using proper JS getters? For template writing, I would find the most concise version:

if (node.roles.has(XYZ)) { doThisOrThat() }

more idiomatic than the more formal but somewhat Java-ish version:

if (node.getRoles().has(XYZ)) { doThisOrThat() }

So I've removed the $node object

I left that object accessible as part of the context for those corner cases where a client might need some properties or method calls of the Opal node object for which we don't provide a JS wrapper. I think we should keep it for now.

@hsablonniere I understand your concerns Hubert, but I would still push toward using the Set type.

Historically, we used object and array for almost every data structure since this was all we have at hand. But using the proper ECMA-262 Set() types may avoid having to manually check some conditions. For example, when using arrays, every line of code adding a new role to a node should explicitly check if that role is not already present to avoid setting it twice. With a Set(), the invarient that each role is present only once is garanteed by the data type.

Concerning your particular use case of "JSON stingifying" a set, you could write something like that (tested on Node.js):

> s = new Set([1,2,3])
Set { 1, 2, 3 }
> JSON.stringify([...s])
'[1,2,3]'

[ggrossetie]

I left that object accessible as part of the context for those corner cases where a client might need some properties or method calls of the Opal node object for which we don't provide a JS wrapper. I think we should keep it for now.

Actually you can already do that with the node object. The wrapper API only adds methods to the base object so even if Asciidoctor.js doesn't provide a dedicated function you can still access all the methods defined in Asciidoctor core.

Opal is using $method_name or attribute_name. Here is a basic example:

class User
  attr_accessor :name

  def initialize(name)
    @name = name
  end

  def admin?
    @name == 'Admin'
  end
end

user['$admin?']() // will call the method "admin?"
user.name // will get the name of the user using the "attr_accessor"

Speaking of 2.x roadmap, can't we consider using proper JS getters? For template writing, I would find the most concise version

We cannot do that otherwise we will need to create/extend the base object. The reason is that Opal will define attributes on the object for attr_accessor. So if we redefine attributes on the object it will conflict.

[ggrossetie]

@s-leroux I've reverted all the "style-related" changes

[s-leroux]

Actually you can already do that with the node object. The wrapper API only adds methods to the base object

Indeed. Let me take the time to review that in more details. And if that's OK, I will resolve the conflicts and merge your changes after that.

[s-leroux]

@Mogztter I find some inconsistency between our two implementations of the "attributes" getter for nodes.

For that basic fragment:

[.Role]
Text

getAttributes() as provided by asciidoctor.js returns:

{ '[object Object]': undefined, role: 'Role' }

Whereas my own naive implementation returned:

{ role: 'Role' }

Does that "'[object Object]': undefined" entry has some meaning?

[ggrossetie]

you mean getAttributes() ? because getAttribute() (singular) and getAttributes() (plural) are not the same functions.

getAttribute is using the attr method:

https://github.com/asciidoctor/asciidoctor/blob/9aefdeed07b2a72b7518c80b1e67239b433f6018/lib/asciidoctor/abstract_node.rb#L93-L97

getAttributes is using the attributes attribute: https://github.com/asciidoctor/asciidoctor/blob/9aefdeed07b2a72b7518c80b1e67239b433f6018/lib/asciidoctor/abstract_node.rb#L11

getAttribute without a value will return the default value Opal.nil, so I guess [object Object] is the object Opal.nil.

[s-leroux]

Indeed, plural.

I suspect this is coming from the object entry in $$keys. In that example, I attached a role and an arbitrary attribute attr to a block:

> d = asciidoctor.load("[.Role,attr=value]\nPara")
> b = d.blocks[0]
> b.attributes
$Hash {
  '$$smap': { attr: 'value', role: 'Role' },
  '$$map': { '3': { key: 1, key_hash: 3, value: '.Role' } },
  '$$keys': [ { key: 1, key_hash: 3, value: '.Role' }, 'attr', 'role' ],
  '$$none': undefined,
  '$$proc': undefined }
> b.getAttributes()
{ '[object Object]': undefined, attr: 'value', role: 'Role' }

As for myself, the expected output would be:

{ attr: 'value', role: 'Role' }

Maybe you should filter out non-string keys when mapping a $hash to a JS object? Or eventually returning a ECMA-262 Map if object keys are associated with meaningful entries that should not be lost?

[ggrossetie]

This is weird, could you please try to debug to get more information on this object ?

Maybe you should filter out non-string keys when mapping a $hash to a JS object? Or eventually returning a ECMA-262 Map if object keys are associated with meaningful entries that should not be lost?

Why not but first I want to know what is this object and why it's here... :thinking:

[s-leroux]

@Mogztter I tracked back the issue to that line:

https://github.com/asciidoctor/asciidoctor/blob/9aefdeed07b2a72b7518c80b1e67239b433f6018/lib/asciidoctor/attribute_list.rb#L183

      @attributes[index + 1] = resolved_name

It maps positional arguments with their index.

In the JS code, this is translated as:

          $writer = [$rb_plus(index, 1), resolved_name];
          $send(self.attributes, '[]=', Opal.to_a($writer));

However, the $rb_plus(index, 1) expressions leads to a number instance. If I understand it correctly, the expected output should be:

{ '1': 'Role', attr: 'value', role: 'Role' }

But []= failed to convert number instances to string like the standard JS Array object does (a[1] === a['1']). I wasn't able to pinpoint where exactly is defined the code corresponding to []= though.

[s-leroux]

@Mogztter See https://github.com/asciidoctor/asciidoctor.js/pull/491 for a possible patch regarding positional arguments (and more generally, numbers used as key in a hash)

[ggrossetie]

Awesome thanks for taking the time to find the root cause 👍

[ggrossetie]

This is neat 👍