-
Notifications
You must be signed in to change notification settings - Fork 97
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
base: master
Are you sure you want to change the base?
Conversation
…uded and use an anchor
@ggrossetie Let me know if you want me to make a separate issue to tie this to |
let textDocumentExt = path.extname(textDocumentUri.fsPath) | ||
const textDocumentName = path.basename(textDocumentUri.fsPath, textDocumentExt) | ||
textDocumentExt = textDocumentExt.startsWith('.') ? textDocumentExt.substring(1) : ''; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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 documentThe changes made:
docname
is set, themacros
substitution is properly able to handle the xref macros when they are referencing the same document that isinclude::
'ing themUNSAFE
as it is in the preview code, this wayincludes::
are actually resolvedresolves #425