Skip to content
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

Introduce an async convert #644

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Introduce an async convert #644

wants to merge 1 commit into from

Conversation

ggrossetie
Copy link
Member

  • The Reader class in Asciidoctor core has a new method read_include_content
    • If a VFS is defined, read the content of the include target from it. Otherwise use the file system or the network to read the content.
  • The convertAsync function will fetch the content of all the unconditional includes and populate the VFS.
  • If an include processor is defined for the target, the content will not be fetched
  • If the target already exists in the VFS skip (cache)

Performance wise it about the same speed for local files but since the event loop is not blocked (or at least less blocked), the performance should be better on a running application.

I think we should be careful about how many asynchronous remote calls we can make at once. We may need to set a limit to avoid resource exhaustion.

Using a cache greatly improve performance if a remote file is present more than once in the document. For reference open-uri/cached module used in Asciidoctor Ruby is not available in JavaScript.


# If a VFS is defined, Asciidoctor will use it to resolve the include target.
# Otherwise use the file system or the network to read the file.
def read_include_content inc_path, target_type
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mojavelinux preprocess_include_directive method is almost identical, I've just replaced File.open with the new read_include_content method.

@mojavelinux
Copy link
Member

I think we should be careful about how many asynchronous remote calls we can make at once. We may need to set a limit to avoid resource exhaustion.

It's annoying that Node.js doesn't provide this out of the box. I run into the same problem whenever I switch to async operations.

@mojavelinux
Copy link
Member

I'm not sure that overriding/importing large chunks of Asciidoctor core is the right idea here. But I'm also still at a loss for exactly how to handle this. We don't really have a true preprocessor that avoids side effects in Asciidoctor, and the include directive is inherently synchronous. This is topic I think we need to consider in the AsciiDoc Language project, then carry out whatever we decide after the architecture is worked out there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants