-
-
Notifications
You must be signed in to change notification settings - Fork 384
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
Refactor: Isomorphic #109
Refactor: Isomorphic #109
Conversation
Conflicts: packages/languages/README.md packages/languages/data/jinja-html.tmLanguage.json packages/languages/data/jinja.tmLanguage.json packages/languages/data/jsx.tmLanguage.json packages/languages/data/svelte.tmLanguage.json packages/languages/data/文言.tmLanguage.json packages/languages/package.json packages/languages/src/lang.ts packages/shiki/languages/jinja-html.tmLanguage.json packages/shiki/languages/jinja.tmLanguage.json packages/shiki/languages/jsx.tmLanguage.json packages/shiki/languages/svelte.tmLanguage.json packages/shiki/languages/文言.tmLanguage.json packages/shiki/package.json packages/shiki/src/highlighter.ts packages/shiki/src/languages.ts packages/themes/README.md packages/themes/package.json
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've given this a clone locally and run through a lot of the changes.
I think this is really well thought PR, and we should merge it. I don't know how free @octref is, so I will give this at least till the end of January for his feedback. If we don't get any, I'll merge this, move the TypeScript website to it and if that all goes well then I'll ship the npm module.
After this PR, shiki should ideally be at v0.9.x - with shiki in the browser as doable then I don't know what else should be blocking hitting 1.0 and getting it well documented for others to use.
Can you add Like here: Also, what if I don't want to use a CDN? do you have an example of the usage? is it expected we would load the file ourselves and pass the resulting object in for a theme for example? Could you possibly allow us to pass a path in to a theme/lang which the loader can then deal with? edit: looks like it will handle being passed a path but will warn about the CDN being missing if i read it right edit2: you also rely on |
packages/shiki/src/registry.ts
Outdated
public async loadTheme(theme: Theme | IShikiTheme | string) { | ||
if (typeof theme === 'string') { | ||
if (!this._resolvedThemes[theme]) { | ||
this._resolvedThemes[theme] = await fetchTheme(`themes/${theme}.json`) |
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.
presumably this means if we want to load a theme by name, it must always exist in themes/
which often won't be true. can it be changed to allow us to specify the exact path ourselves?
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.
You can call the loadTheme
API first and then passed the resolved theme object to it. You can also use your own loader to load your grammar/theme json and pass them to shiki, this is how you use it without CDN.
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.
do you not think it would be a quick win to allow that to be deal with by the loader too? you already have the logic for CDNs and it would be identical logic almost for your own paths. it'd save consumers having to write pretty much the same logic in their codebases. just a thought.
or even allow themes/
to be some configurable root path, and the same for the others
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.
Not sure if I got your points, can you show me some examples? Thanks.
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.
this is making a strong assumption that themes and languages always exist at themes/
and whatever directory you ship languages in.
but this isn't true for many consumers due to bundling. it is increasingly likely that projects will split bundles and use dynamic imports these days, which means its possible the themes may well exist in production as individual files but the bundle which loads them may no longer be relatively in the same place it was in this repo.
so my suggestion would be one of the following:
// ideal but difficult for you because you wouldn't know which string is a path, and which is a theme name except by pattern
loadTheme('./my-own-theme.json');
// alternatively, make the assumed path configurable
registry.themesRoot = '/foo/bar';
loadTheme('test'); // results in '/foo/bar/test.json'
of course where themesRoot
exists in config, so wherever you initialise shiki.
otherwise, this means your current code is fine but anyone who doesn't ship this package exactly as-is will have to manage loading by themselves (of assets). which seems a shame given how small an improvement it may be
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.
for example, one of our main repos uses webpack but does not use non-standard imports, i.e. it sticks to native supported imports. which means no magical webpack imports (like json imports, etc).
in our case, we would ship the languages we want in an assets directory somewhere and load them up from the bundle which will be elsewhere.
in that case we'd have to deal with the loading manually to avoid your hard coded themes/
path. so it would be nice to just tell shiki where to find the themes, or just give a path to the theme we want.
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.
Yeah, that makes sense. We can have a logic to detect if it's a built-in theme or relative/absolute path. While I don't have much bandwidth on this atm, would you mind opening a PR against my fork so the changes can be reflected here? Thanks.
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.
No worries, I'll have a look through it soon and see what I can come up with
import * as Onigasm from 'onigasm' | ||
import type { IOnigLib, IRawGrammar, IRawTheme } from 'vscode-textmate' | ||
import type { IShikiTheme } from './types' | ||
import { dirname, join } from 'path' |
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.
out of interest, how does this work in browsers? is rollup or something stripping it?
One last note is that your ESM output will not be consumable by browsers, which is a shame seeing as that is the way forward, not IIFEs. If you could output an actual ESM bundle, that would be so nice. It wouldn't be a big change to your rollup file, it just means bundling it with all externals. So we can do this: <script type="module">
import {getHighlighter} from 'shiki'; // assuming we setup an import map already
// do stuff
</script> this is also where |
Agreed that we can have a browser ESM build |
@octref one last ping, I'll give this one last look over next week (deal with conflicts etc) and merge otherwise |
FYI i did open a PR against this PR with the suggested path option at antfu#1 that and a browser ES module (it could be a bundle still, just in esm format) would be pretty useful to have, if possible |
Oh sorry, @43081j I didn't see it. Somehow GitHub doesn't watch for forks automatically. I will have a look later and get back to you. Will solve the conflicts as well. (the main branch has a GitHub action that fetches the file updates which cause the conflicts, shouldn't be a big problem tho) |
no worries, if you have a better approach in mind just let me know and ill rework it if needed 👍 |
Alright, tests pass and things seem to build fine - I've given the diff another read over and I think this is good to go. |
@antfu did this end up containing a browser esm build? or will it have to be done in another PR? |
Following this RFC: #91
DEMO: https://codepen.io/antfu/pen/zYKNeWN
Try out now:
npm i @antfu/shiki
orhttps://unpkg.com/@antfu/shiki
Changes
shiki-languages
andshiki-themes
get deprecated, languages and themes is now shipped with theshiki
packagegetHighlighter()
internal changed from class to functionalthemes
allowing to load multiple themes asynchronously on initialization and being able to use them synchronously later.renderToHTML()
andcodeToThemedTokens()
support a third argument for specify the themeasync loadLanguage()
andasync loadTheme()
to load the resources incrementallygetBackgroundColor()
andgetForegroundColor()
API.shiki
shiki-renderer-svg
will extend theshiki
variable and available atasync shiki.getSVGRenderer()
setCDN()
lerna publish
to do the version bumping and release (all packages will be the same version)plist
support as they are already normalized to jsonBREAKING CHANGE
codeToThemedTokens()
options moved from the third arg to fourth, leaving the third for specifyingtheme
langs
option ingetHighlighter()
is specified, it will no longer include all the bundled languages, uselangs: [...BUNDLED_LANGUAGES, myCustomLanguage]
to have the previous behaviorloadTheme
is now async