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

Fix document to document xrefs in preview when the documents are included #872

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

birdman7260
Copy link

Refer to my comment made on the issue for details about the issue and resolution:
#425 (comment)

Resolves: in a preview, when a document xref's another document and then that document include::'s the first document

The changes made:

  • src/asciidocEngine.ts
    • added the intrinsic attributes that were missing. These are missing because they are only intrinsically assigned when asciidoctor processes a file but we process with the input as a string. Reference the asciidoctor documentation, notice that the modifiable column calls out that these are only assignable via the API in this situation: Support xref navigation in the preview #425 (comment)
    • Now that docname is set, the macros substitution is properly able to handle the xref macros when they are referencing the same document that is include::'ing them
  • src/test/asciidoctorWebViewConverter.test.ts
    • refactored the two types of conversions to utilize a new function that creates the converter options. This applies the same intrinsic attributes solution
    • also, this sets the safe mode to UNSAFE as it is in the preview code, this way includes:: are actually resolved
    • Added two tests: 1) test that the included file is able to xref into the parent file, 2) new test to make sure an included file can xref a separate included file

resolves #425

@birdman7260
Copy link
Author

@ggrossetie Let me know if you want me to make a separate issue to tie this to

Comment on lines +164 to +166
let textDocumentExt = path.extname(textDocumentUri.fsPath)
const textDocumentName = path.basename(textDocumentUri.fsPath, textDocumentExt)
textDocumentExt = textDocumentExt.startsWith('.') ? textDocumentExt.substring(1) : '';
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this code in https://github.com/asciidoctor/asciidoctor-vscode/blob/master/src/asciidocTextDocument.ts

Since baseDir is not available in a browser environment (such as https://github.dev) it would be better to handle this case in one location.

Copy link
Author

Choose a reason for hiding this comment

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

I could move that, but I tried to test things in the browser environment and was running into issues.
Is it a known problem that the include directive does not work in the browser environment? Without includes working my fix doesn't actually apply. I tried to use https://github.dev can created a quick example of two adocs and one including the other and it printed out the Unresolved directive text in the preview.
Extensions in general are new to me, but I could not figure out how to get it to work, even when I set the base_dir according to https://docs.asciidoctor.org/asciidoctor.js/latest/spec/include-support-matrix/
When the runtime is a browser the browser compiled version of asciidoctor is used and it uses XMLHttpRequest to try to open the includes and i just can't figure out how to get that to work correctly... seems the VSCode extension documentation wants all file accesses to use vscode.workspace.fs which makes sense. We could probably set up our own inludeProcessor like you've done with Antora but I really don't know anything about Antora and whether or not that works in the browser environment. I think it would still be possible to do, might even be nice to make our own includeProcessor that works no matter the situation?

That's a fair bit beyond what i can do at the moment though, I see yours is creating Opal classes and whatnot and I'm just barely hangin on when it comes to Opal.

Copy link
Member

Choose a reason for hiding this comment

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

Is it a known problem that the include directive does not work in the browser environment?

In theory, it could work if the File System API abstraction provided by VS code can read file in a browser environment.
I don't recall if include are currently supported or not in a browser environment.

My point is that we should not execute code that relies on Node.js path or fs module since they won't necessarily be available in a browser environment and might throw an error.

For now, you can use 'browser' in process && (process as any).browser === true to test if we are running in a browser environment and not call path or fs module.

I would create a ComputeIntrinsicAttributesDocumentAttributes in asciidocTextDocument.ts which can return an empty object (or undefined) when running in a browser environment. Does it make sense?

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

Successfully merging this pull request may close these issues.

Support xref navigation in the preview
2 participants