Fix: fetchTheme theme 'include' directory joining for absolute paths

[Oblongmana]

Combo bug report/pull request here - please review carefully when you can, I definitely may have missed things, and I've only examined this fix in the context of using it in a VSCode plugin, and in the node REPL - I haven't tested it in browser etc. I've written some extra tests too. Note though that I'm assuming loadTheme is only meant to work with absolute URLs - see my extra notes at the bottom of this post. Apologies for the wall of text!

I was using shiki to load vscode themes using absolute pargs from my local file-system, and if I called shiki.loadTheme (i.e. loader.fetchTheme) with an absolute path for a theme with an 'include' property referring to a second theme, it would try to load the second theme using only the last directory in the path of the first theme, combined with the 'include' property - e.g. if first theme was in /foo/bar/baz/bing.json and had an include for ./buzz.json, it would think the path for the second file was baz/buzz.json due to the call (join(dirname(themePath), shikiTheme.include).

So because fetchTheme would say the path for the included theme was baz/buzz.json, this would get to the _resolvePath function, it would think it was a relative path, and return path.resolve(__dirname, '..', filepath); - giving a very wrong path - in this case, assuming I'm running in /project, it would give /project/node_modules/shiki/bar/buzz.json.

An example below, where I ran this in the node REPL (having already yarn added shiki) to import the light_plus theme, which includes the light_vs theme. Should be easy to repro, assuming you are running Windows and have VSCode installed - where I'm trying to load the theme from (this is just an example - I know some of these themes are included in shiki itself!)

$ node
> shiki = require('shiki')
> shiki.loadTheme("c:\\Users\\james.000\\AppData\\Local\\Programs\\Microsoft VS Code\\resources\\app\\extensions\\theme-defaults\\themes\\light_plus.json");
Promise { <pending> }
> (node:37452) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, open 'C:\Users\james.000\repositories\vscodeExts\salesforce-doc-lookup\node_modules\shiki\themes\light_vs.json'

Relative URLs

As far as I can tell, calling shiki.loadTheme doesn't work with with relative URLs? If I try to follow the instructions in themes.md to use a relative parg, it always seems to treat them as relative to the node_modules/shiki directory, regardless of where I run them from - likely due to the _resolvePath function prepending them with __dirname. I haven't touched that functionality at all - possibly it might be worth specifying in themes.md that custom themes must always be absolute URLs, and maybe adding a guard against it? Probably depends on whether it's meant to handle relative urls or not

[matthewjamesadam]

Hi there, I have run into the same issue this PR resolves -- specifically, loading themes directly from VSCode installations does not work because of the relative path issues outlined here. Is there any chance this PR could be merged?

[netlify]

✅ Deploy Preview for shiki-matsu ready!

Name	Link
🔨 Latest commit	c26c14d
🔍 Latest deploy log	https://app.netlify.com/sites/shiki-matsu/deploys/62f6ff4bb6c0a30008b06f2d
😎 Deploy Preview	https://deploy-preview-173--shiki-matsu.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[orta]

👋🏻 sorry for the super late review, trying to get a sense of why this is marked as draft?

I think supporting relative paths is a good call (think I hit this also) and your implementation is thoroughly tested and looks good to me

[Oblongmana]

👋 Hey @orta - I've forgotten exactly why I marked it as draft, I think! 🙈 Reading my notes, I think I felt I'd perhaps not manually tested thoroughly enough - not enough in the browser perhaps, just in the VSCode extension context I was hacking around in?

I've removed the draft flag so it's marked ready for review. Getting late here, but I'll take a look in at those test failures tomorrow, though at a glance I'm not sure they're related to this PR? 🕵️

[orta]

Yep, I bet a rebase will fix it with #335

[Oblongmana]

Nearly there - rebased, but looks like json5 (which the new fetchtheme.test.ts used) has been replaced with jsonc-parser. Will take a look into it today 🕵️‍♂️

[Oblongmana]

@orta Rebased, and updated that test - looks like we're good to go! Let me know if you need anything else 🙏

[orta]

👍🏻 thanks!