asciidoctor / gulp-asciidoctor

gulp-asciidoctor
https://www.npmjs.com/package/@asciidoctor/gulp-asciidoctor
14 stars 8 forks source link

Fixed issue #15 Upgrade to Asciidoctor 2.2.0 #17

Closed henriette-einstein closed 4 years ago

henriette-einstein commented 4 years ago

@Mogztter I get an error if I try to run the test case with a converter that is exported as a class. I don't know, why that is the case. Do you have any idea?

It says: convert: undefined method `convert' for # even though I explicitly checked for the existence of the function before.

When I export the class via a function, it works.

ggrossetie commented 4 years ago

@henriette-einstein Strange... we do have a test on this feature: https://github.com/asciidoctor/asciidoctor.js/blob/cd07a68127eff5cd080e9926c0a85ff388fcadc8/packages/core/spec/node/asciidoctor.spec.js#L2386 and the class definition is here: https://github.com/asciidoctor/asciidoctor.js/blob/cd07a68127eff5cd080e9926c0a85ff388fcadc8/packages/core/spec/node/asciidoctor.spec.js#L2175-L2202

Can you please share a sample reproduction case? And/or add a failing test?

henriette-einstein commented 4 years ago

There is a test case included in this branch, that fails when you run "yarn test". It is the same as in the previous release (the master branch). It worked with Asciidoctor 2.1.2.

To reproduce, you can just checkout this branch and run "yarn test" or "npm test".

I can isolate that test and add some more stuff if that will help. Just give me notice.

henriette-einstein commented 4 years ago

I just saw, that you are passing in a class. In my testcase, I create a new instance using new(). I'll check if that behaves the same as in your case.

henriette-einstein commented 4 years ago

I tweaked the code a little for debugging. Now I get the following "stacktrace"

 convert: undefined method `convert' for #<Proc:0x6b6>
  at Function.$method_missing (node_modules/asciidoctor-opal-runtime/src/opal.js:3907:56)
  at Function.method_missing_stub (node_modules/asciidoctor-opal-runtime/src/opal.js:1310:35)
  at constructor.$convert (node_modules/@asciidoctor/core/dist/node/asciidoctor.js:8418:35)
  at Function.$convert (node_modules/@asciidoctor/core/dist/node/asciidoctor.js:16304:22)
  at Function.Asciidoctor.convert (node_modules/@asciidoctor/core/dist/node/asciidoctor.js:21693:21)
  at Transform._transform (index.js:86:28)
  at Transform._read (_stream_transform.js:191:10)
  at Transform._write (_stream_transform.js:179:12)
  at doWrite (_stream_writable.js:385:12)
  at writeOrBuffer (_stream_writable.js:367:5)
  at Transform.Writable.write (_stream_writable.js:307:12)
  at Context.<anonymous> (spec/gulp-asciidoctor.spec.js:393:12)
  at processImmediate (internal/timers.js:456:21)

Another info: I added another testcase (it('should register a custom converter instantiated here',) in which I instantiate the converter in the test case itself (line 406). That works. If I pass the class and instantiate it in index.js, the test fails. (This can be reproduced if you change line 50 in index.js to

asciidoctor.ConverterFactory.register(new options.converter(), [asciidoctorOptions.backend])

and line 406 of gulp-asciidoctor-spec.js to

  converter: CnvClass
ggrossetie commented 4 years ago

@henriette-einstein Thanks I will checkout your branch tomorrow to try to understand why it's failing.

henriette-einstein commented 4 years ago

I found it! That is probably a pathological edge-case.

What happened is basically

const Clazz = MyClass
const myInstance = new Clazz()
asciidoctor.ConverterFactory.register(myInstance, [asciidoctorOptions.backend])
asciidoctorOptions = {
   converter: Clazz
}
asciidoctor.convert(content, asciidoctorOptions)

In that case, I have a class that is instantiated and registered before the class itself is passed as an option to the asciidoctor processor.

I don't know if such a case could be checked in your code.

I removed the option "converter" after the registration. Then it works.

Sorry for the disturbances, it was my fault.

henriette-einstein commented 4 years ago

I did not know, that "converter" was actually an option that is used by the underlying Asciidoctor. I therefore changed the name of the plugin option to "cnv" to avoid problems.

ggrossetie commented 4 years ago

I did not know, that "converter" was actually an option that is used by the underlying Asciidoctor. I therefore changed the name of the plugin option to "cnv" to avoid problems.

Yes converter is an option but if you do that you will need to pass a "Ruby" class or object. The asciidoctor.ConverterFactory.register function will "lift" the JavaScript class or instance to an Opal class/object.

ggrossetie commented 4 years ago

@henriette-einstein The code looks fine, I only have minors nitpicks about the documentation, otherwise :+1:

henriette-einstein commented 4 years ago

@Mogztter I am ready to merge the pull request if that is ok for you

ggrossetie commented 4 years ago

@henriette-einstein It looks good to me, I think you can merge it! Well done :+1:

ggrossetie commented 4 years ago

:tada: :tada: :tada: