fix: use URI.fsPath to get valid Windows paths #2
Conversation
| @@ -119,36 +119,36 @@ export class DocsWorkspace implements IWorkspace { | |||
| } | |||
|
|
|||
| hasMarkdownDocument(resource: URI) { | |||
| const relativePath = path.relative(URI.file(this.root).path, resource.path); | |||
| const relativePath = path.relative(path.resolve(this.root), resource.fsPath); | |||
There was a problem hiding this comment.
One change I made here was to just use path.resolve to get an absolute path for the root dir. I think that's easier to reason about than URI's handling of relative paths, but I'm happy to switch this to URI.file(this.root).fsPath instead if that's preferred.
dsanders11
changed the title
|
Thanks @itsananderson. LGTM, but I'm going to hold off on merging for a few days and hopefully get the npm package issues resolved so it will actually publish on merge. |
There was a problem hiding this comment.
LGTM.
Following @dsanders11's lead from yesterday of not hitting "approve" since he mentioned wanting to hold off on merging this for a few days, and if this shows up as green it might get merged by accident
itsananderson
force-pushed
the
fix-windows-paths
branch
from
731c5f3
to
e844c5e
Compare
|
🎉 This PR is included in version 1.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
After some debugging, I've figured out the Windows issue I've been encountering.
Calling
URI.pathfor afile://URI returns a string that starts with a leading slash, so for example:Would become:
That path is not a valid Windows path, so calls to
fs.existsSyncetc. will fail even though that target Markdown file exists on disk.After digging around a little in
vscode-uri, I found that they have a separate fsPath property which seems to behave how we actually want it to behave, returning a string like:While debugging this, I also found that there aren't actually any test fixtures for valid markdown doc links (which explains why the tests were green on Windows). I added one test fixture for a valid cross-doc link to help verify that this actually fixes something.