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.

Sign up for GitHub

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

Jump to bottom

Optional include inside an include file raises an Unresolved directive #1580

Open
someth2say opened this issue Feb 7, 2022 · 3 comments
Open

Optional include inside an include file raises an Unresolved directive #1580

someth2say opened this issue Feb 7, 2022 · 3 comments
Labels
🐛 bug 🍩 improvement 🌐 Web

Comments

@someth2say
Copy link

Say we have a root file with an include directive:

Root

include::test.adoc[]

Then, the test file:

Test

include::test2.adoc[opts=optional]

The expected result was just the text from both files (Root and Test), but I get a Unresolved directive error:
image

Given the second include is optional, the error should not appear.
@ggrossetie ggrossetie transferred this issue from asciidoctor/asciidoctor-browser-extension Feb 7, 2022
@ggrossetie
Copy link
Member

This feature is not implemented in a browser because it relies on File.file? which is not available in a browser environment:
https://github.com/asciidoctor/asciidoctor/blob/4d849f85a7e2684c0553c8d9082c5b82ead2cfbc/lib/asciidoctor/reader.rb#L1236-L1245

In other words, we cannot check that the file exists without doing an XMLHttpRequest.
I guess we could reverse the logic and perform an XMLHttpRequest only if optional-option is present since we are already monkey-patching the resolve_include_path:

asciidoctor.js/packages/core/lib/asciidoctor/js/asciidoctor_ext/browser/reader.rb

Lines 3 to 39 in bb39bb2

def resolve_include_path target, attrlist, attributes
# NOTE when the IO module is xmlhttprequest, the only way to check if the file exists is to catch a 404 response
p_target = (@path_resolver ||= PathResolver.new '\\').posixify target
target_type, base_dir = :file, @document.base_dir
if p_target.start_with? 'file://'
inc_path = relpath = p_target
elsif Helpers.uriish? p_target
unless (@path_resolver.descends_from? p_target, base_dir) || (@document.attributes.key? 'allow-uri-read')
return replace_next_line %(link:#{target}[#{attrlist}])
end
inc_path = relpath = p_target
elsif @path_resolver.absolute_path? p_target
inc_path = relpath = %(file://#{(p_target.start_with? '/') ? '' : '/'}#{p_target})
elsif (ctx_dir = (top_level = @include_stack.empty?) ? base_dir : @dir) == '.'
# WARNING relative include won't work in the Firefox web extension (unless we use the second line)
inc_path = relpath = p_target
#inc_path = %(#{File.dirname `window.location.href`}/#{relpath = p_target})
elsif (ctx_dir.start_with? 'file://') || !(Helpers.uriish? ctx_dir)
# WARNING relative nested include won't work in the Firefox web extension if base_dir is '.'
inc_path = %(#{ctx_dir}/#{p_target})
if top_level
relpath = p_target
elsif base_dir == '.' || !(offset = @path_resolver.descends_from? inc_path, base_dir)
relpath = inc_path
else
relpath = inc_path.slice offset, inc_path.length
end
elsif top_level
inc_path = %(#{ctx_dir}/#{relpath = p_target})
elsif (offset = @path_resolver.descends_from? ctx_dir, base_dir) || (@document.attributes.key? 'allow-uri-read')
inc_path = %(#{ctx_dir}/#{p_target})
relpath = offset ? (inc_path.slice offset, inc_path.length) : p_target
else
return replace_next_line %(link:#{target}[#{attrlist}])
end
[inc_path, :file, relpath]
end

@mojavelinux
Copy link
Member

Another idea is to catch the exception when the request for the file fails and suppress the behavior if the optional option is set. I haven't looked to see if there is an opportunity in the code to handle it that way. That's just the idea in theory.

@ggrossetie
Copy link
Member

Another idea is to catch the exception when the request for the file fails and suppress the behavior if the optional option is set. I haven't looked to see if there is an opportunity in the code to handle it that way.

Technically it's possible but not practical since we would need to monkey patch at different locations:

Asciidoctor eagerly check that the file exists (using File.file? in resolve_include_path) to avoid additional processing.
We could probably send a HEAD request if optional-option is present... in my opinion that's the lesser evil 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🐛 bug 🍩 improvement 🌐 Web
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mojavelinux @ggrossetie @someth2say