Suggestion: alternate syntax for language loading

[drwpow]

👋 Hello! I’m a huge fan of this project and I’m excited for the direction this is taking. I’ve implemented Shiki in a number of different setups and wanted to suggest a possibly alternate approach to setCDN() as that’s been my biggest sticking point. Please close this issue if this suggestion is out-of-place or not the direction you’d want to take.

Problem

Shiki implementing its own fetch() mechanism and requiring a specific folder structure only works for specific environments, but makes it cumbersome when dealing with SSR, CloudFlare workers, or other setups. See the number of open issues filed around this:

• Unclear (possibly incorrect) instructions to register a new language #317
• CloudFlare workers support #247
• [Help] Can't make it work in SvelteKit #242
• [Shiki] no CDN provider found #216

Proposal

Now, the solution to all of these may just be “we’re holding it wrong” and additional docs or clarification are needed. But just for discussion, what if the following mechanism were used instead to load languages:

  shiki.getHighlighter({
    theme: 'nord',
    langs: [
      {
        id: 'rockstar',
        scopeName: 'source.rockstar',
-       path: './rockstar.tmLanguage.json' // or `plist`
+       import: () => import('./rockstar.tmLanguage.json')
      }
    ]
  })

It’s a tiny change, but it would mean that no internal fetch() mechanism would be needed, nor would that need to be re-implemented for SSR, nor mocked in testing, etc. etc. Now that import() is a part of the language spec, it passes off responsibility to the platform to handle loading that module, and it can make better optimizations around this than shiki can (also, if a person still wanted to continue using the CDN structure in a /public/ directory this would let them use it the same way—both local and remote URLs are supported by import()).

Of course, the outlier is the oniguruma Wasm file. But I believe the fetch() still wouldn’t be needed as only a URL is required, and JS itself can handle the rest:

  shiki.getHighlighter({
    theme: 'nord',
    wasmURL: import.meta.env.WASM_URL, // "/assets/oniguruma.wasm"
  })

This would also let the build tool better optimize this, and even pass environment-dependent values for this Wasm URL (and I think by this point many people understand that an absolute URL is required, and also build tools like Vite allow management of this through ?url and ?worker imports).

Summary

• path string replaced with async import() (wrapped in a func so Shiki controls when this fires)
• No internal fetch()
• Expose Wasm URL in options

My belief is that this would simplify Shiki and make it more flexible to use on more platforms. But is that the case? Am I missing something? Would love some holes poked in this theory. Thanks for reading! 🙂

[babakfp]

So, this was working 5min ago, but it doesn't anymore :)
https://sveltekit-shikijs.vercel.app
https://github.com/babakfp/sveltekit-shikijs

[octref]

Sorry for my late response. Been busy with other things. Love the proposal. Now that we are working towards 1.0 it's ok to break APIs. Freel free to send in a PR 🚀

[tom-sherman]

Wasm URL wouldn't be enough in Cloudflare Workers, you can't fetch and execute dynamic code in that environment. We'd need to be able to pass a WebAssembly.Module to getHighlighter.

[tom-sherman]

Another change which I think would make sense along with this is removing the idea of "bundled languages".

This proposal makes shiki more isomorphic, and "bundled languages" means totally different things depending on the enviroment - so I think it makes sense to remove it. It can always be added back in later with functions like:

getHighlighter({
  langs: nodeJsBundledLangs(),
  // or
  langs: cdnBundledLangs("https://example.com/cdn"),
})

This could even be inferred based on the environment to ease uprading.

[muenzpraeger]

with this is removing the idea of "bundled languages"

Can you specify what you mean with that?

[tom-sherman]

Right now there are a bunch of languages that are assumed to come included with Shiki. These are distributed in the npm package and then resolved at runtime to the textmate grammar JSON.

In a node environment this essentially "just works" but makes other environment require workarounds, or to configure shiki in a specific way via setCDN. The API in this proposal is a more explicit approach, the user declares what the languages are and where they're loaded from. I think then it makes sense to remove the notion of bundled languages entirely, and leave it completely up to the user to provide these, along with a callback to fetch them.

[muenzpraeger]

From my perspective the bundled languages are an important value prop.

They remove the need to search for the "right" language definition, and offer a broad selection.

[tom-sherman]

They can still be included, but in a much more composable way that makes the API the same in all environments. It simply means you have to import the relevant bundled languages and pass them to getHighlighter as shown in my example above.

[muenzpraeger]

Something you can do today, IMHO. DEFAULT_LANGUAGES is in this case an array of tmLanguage files that have been manually read.

const langs = DEFAULT_LANGUAGES.map(
    (bundle) => ({
        id: bundle.id,
        scopeName: bundle.scopeName,
        aliases: bundle.aliases,
        grammar: bundle
     }),
    {}
);

this.highlighterPromise = getHighlighter({
    themes: THEMES,
    langs
});

[antfu]

Coincidentally, I made an exploration with a similar idea as https://github.com/antfu/shikiji, and opened a proposal #510. If you are interested and gave it a try, I'd love to hear any feedback from your usages :)