feat: add support for controlling bundle size #335
Conversation
✅ Deploy Preview for expressive-code ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| * @example { 'mjs': 'javascript' } | ||
| */ | ||
| langAlias?: Record<string, string> | undefined | ||
| // TODO: This should be simply L but for backwards compat, need to support string. This should be changed so that you cannot provide |
There was a problem hiding this comment.
I believe this is a bug in the current released version which uses Record<string, string>, however the languages (the value of the key) should be constrained to the languages available in the current bundle. Currently, you could write an alias of foo->bar and would not get any typescript warning (it would fail at runtime). Additionally, with string, string, you do not get any intellisense/autocomplete. I've adjusted to use StringLiteralUnion which will solve the intellisense problem but I believe this should just be L in order to provide proper type safety. I made this backwards compatible (will support any string) per the goals of the first draft of this PR but my recommendation is that this should be changed to L. It wouldn't be considered a breaking change, I don't believe, because the current version will fail at runtime anyway.
There was a problem hiding this comment.
This was intentionally not limited to the bundled languages as users can add their own languages through the langs option, and it should be possible to define aliases for such additional languages as well. But using StringLiteralUnion is a great idea.
There was a problem hiding this comment.
Ah, I see what you did here now, nice :) Sorry, overlooked the way this was handled previously.
Ok, I just committed 58ec5b5 which makes the following adjustments:
- Removes the TODO
- Adds a note in the
langAliasdoc regardingnameproperty to make it clear how it must align (nameis required on LanguageRegistration but this should help make it clear howlangAliasmatches up) - Aligned the docs for PluginShikiOptions and PluginShikiBundleOptions regarding
langs - Removed langAlias from
PluginShikiHighlighterOptionssince it never really made sense to support it in the first place since the user is under full control of the highlighter and can specifylangAliasdirectly. If we did choose to supportlangAliaswould could only supportL(notStringLiteralUnion<L>) since we don't have alangsproperty to iterate and load from nor do we "init" (and cache) anything like we do withpluginShiki/plugShikiBundle. - Added tests regarding custom languages and aliases since I didn't see any coverage for this scenario in the existing tests.
| async function createRegexEngine(engine: PluginShikiOptions['engine']) { | ||
| // The [...engine...][0] syntax makes it easier to find this code in the built package, | ||
| // allowing astro-expressive-code to remove unused engines from the SSR bundle | ||
| // TODO: This could be adjusted to use direct imports if/when astro-expressive-code is updated |
There was a problem hiding this comment.
If astro-expressive-code is updated to use Core, these can become static imports vs. dynamic
| } | ||
|
|
||
| export type RehypeExpressiveCodeCoreRenderer = RehypeExpressiveCodeEngineRenderer<ExpressiveCodeCore> | ||
| export type RehypeExpressiveCodeRenderer = RehypeExpressiveCodeEngineRenderer<ExpressiveCode> |
There was a problem hiding this comment.
These wrapper types only exist for backwards compat. Ultimately, ec in the base type is the instance of ExpressiveCode/ExpressiveCodeCore that was created but from a typing perspective, I believe it makes more sense to just have ec: ExpressiveCodeEngine vs the actual instance types. If I'm understanding the code, ExpressiveCode class is just a tiny layer on top of the engine. If ExpressiveCode/ExpressiveCodeCore classes are thought of as a bootstrap for the engine, they shouldn't ever have any specific API so exposing the engine type directly would simplify the typings here and in a couple of other places.
There was a problem hiding this comment.
Further thoughts on this at #335 (comment)
|
|
||
| type ShikiDetails = Awaited<ReturnType<typeof buildFixture>> | ||
|
|
||
| describe('integration with shiki affect on ec bundle size', () => { |
There was a problem hiding this comment.
This test suite adds ~25seconds to pnpm test. I think there is a lot of value in having all these tests to inspect/review various configurations but for normal CI runs, possibly narrow this down to a subset of these across the 4 variation groups. Each individual test takes ~1.5sec.
There was a problem hiding this comment.
It's awesome that you've added these tests and agree that there is a lot of value in having them.
In case we need to limit the tests performed during normal runs to prevent slowing down development too much, which subset would you recommend to always run?
There was a problem hiding this comment.
The more that I've thought about this, I think for CI, leaving all the tests makes sense - at least for the main branch workflow, possibly pr branches can have a subset.
Where this additional ~25secs being a challenge is in local development, possibly that is what you meant by "slowing down development"? I think this question really comes down to what the normal workflow is for a developer when testing EC. If the approach is using pnpm test from the root, then this additional time isn't ideal.
I think at minimum, the tests with no-themes.ts would be a good baseline across all 4 test categories within the suite. This makes sure that a vanilla configuration with no additional themes specified works as expected re: bundle size for ec, ec-core, ec-core-bundle and ec-core-highlighter. This could be easily accomplished by adding a parameter to TestCase and then conditionally using test.skip within the test based on its value.
|
This is seriously impressive work! Thank you for diving so deep into this, finding clever solutions, documenting your decisions, adding tests - just wow. I would have never expected this amount of effort being put into limiting the bundle size. I've just returned from holidays and will have a look at this in more detail soon. Based on how substantial these changes are, going through them could take a while, but I'll make sure to make room for it in my schedule. Thanks again! |
Welcome back @hippotastic, hope you enjoyed the time away! Appreciate your kind words, looking forward to hearing your further thoughts once you've had a chance to fully digest. In meantime, I'll start reviewing your feedback thus far and comment back and/or update PR accordingly. |
|
I was able to figure out why Given the goal of this PR is to be 100% backwards compatible, in order to eliminate
A few things:
|
|
|
||
| // NOTE - tsup code splitting is disabled to avoid re-exports being dropped | ||
| // see https://github.com/evanw/esbuild/issues/1737 & https://github.com/evanw/esbuild/issues/1521 | ||
| // TODO: Consider a different bundler in order to support code-splitting to reduce /dist size (e.g., unbuild) |
There was a problem hiding this comment.
I had to disable splitting in expressive-code due to limitations with tsup/esbuild and re-exports. I noticed that you also have it disabled in @expressive-code/core, likely for the same reason.
This isn't ideal but also not the end of the world since the total size of dist doesn't increase all that much but possibly it may be worth considering a bundler that has better support for re-exports such as unbuild which would, more or less, be a drop-in replacement for tsup.
| "target": "ESNext", | ||
| "paths": { | ||
| "@expressive-code/core": ["./packages/@expressive-code/core/src"], | ||
| "@expressive-code/core/hast": ["./packages/@expressive-code/core/src/hast"], |
There was a problem hiding this comment.
After adding core entry points to the three (3) packages, I started getting typescript warnings/errors due to unresolved packages. I only needed to add entries here for the new core modules but noticed that several others were missing so added those in as well. If their absence was intentional, let me know and I can remove all but the new core ones.
Also, along a similar line and while completely unrelated to this PR, I noticed that this line has a typescript error File '/home/barry/repos/expressive-code/packages/rehype-expressive-code/test/utils.ts' is not under 'rootDir' '/home/barry/repos/expressive-code/packages/astro-expressive-code'. 'rootDir' is expected to contain all source files.ts(6059). There are other examples of this error in astro-expressive-code. Is there a reason you explicitly set rootDir and not in tsconfig.base.json?
| const jsModules = await ec.getJsModules() | ||
|
|
||
| return { | ||
| ec, |
There was a problem hiding this comment.
As alluded to in #335 (comment), the more I've thought about this, I think that ec here should be typed ExpressiveCodeEngine. If we make this change, then we do not need the ctor option that gets passed into this function as we can always use ExpressiveCodeCore to create ec even when we're coming from an ExpressiveCode path. If we make that change, then we could also eliminate the loadTheme option and solely rely on customLoadTheme. To do that, in createRenderer, we would adjust to set customLoadTheme: options.customLoadTheme ?? loadShikiTheme.
If we make both of those changes, we can significantly reduce the complexity of the types and "lift" everything in common.ts directly in to core.ts.
The only downsides to make these two changes are:
- It would not be 100% backwards compatible with v0.41.2. However, other than the
typingsthemselves, at runtime, the only place I could see this causing any issues is if someone was doingec instanceof ExpressiveCode, a situation that is highly unlikely to be done by userland. - We would be changing the actual options passed in by the user if they didn't provide a
customLoadThemeso any downstream usage of options would now have acustomLoadThemewhen it originally didn't. I do not think this is a problem though since if they are heading down thecreateRendererpath anyway, the only downstream usage of options is going to be stock functionality anyway.
There was a problem hiding this comment.
Found another opportunity to reduce bundle size as it relates to the default themes that are currently included in ExpressiveCodeEngine.
In short, when themes are provided in the config, the github-dark and github-light themes are still included in the bundle even though they will never be used. This unnecessarily increases bundle size by 22,472 bytes.
To eliminate the increase when themes are provided, there are three options:
- Eliminate these defaults from
ExpressiveCodeEngineand move them to a higher level of abstraction (e.g.,ExpressiveCodeitself) and throw an error inside of ExpressiveCodeEngine if no themes have been specified (it would fail a little father down in the code path anyway so an error gives something meaningful vs. an unhandled exception). The only downside to this approach that I can see is that if anyone in userland is creating an instance ofExpressiveCodeEnginedirectly, this would be a breaking change. My guess is it's unlikely anyone is creating a direct instance of the engine, however, and even in that case, it would be a simple adjustment to their code to include their desired themes. - Create a new
ExpressiveCodeEngineCoreand expose via a different entry point. In short,ExpressiveCodeEnginewould be a subclass ofExpressiveCodeEngineCoreand layer in the default themes before callingsuper. This approach would only be required to ensure 100% backwards compat but introduces an additional layer of complexity to the code base. I've created an example of what this would look like. Note that the name@expressive-code/core/corefor the entry point likely needs a different name (e.g., possiblybase). - A variant of option 2, this would instead be to create a different entry point (e.g.,
full) that contains everything in@expressive-code/corethat isn't actuallyCore- for now, this would just beExpressiveCodeEngine. Given the namecoreis already in the package name andCoreis implied, this make a little more sense than option 2 but it does meanExpressiveCodeEnginewould no longer be exported via@expressive-code/coreand instead@expressive-code/core/fullwhich would be a breaking change for anyone consumingExpressiveCodeEnginedirectly.
At the end of the day, 22,472 bytes isn't the end of the world but it's still unnecessary in cases where userland wants full control of the bundle size by utilizing the Core layer. I think option 1 is the most straightforward with only one likely minor drawback. That said, option 2 & 3 provide some additional opportunities for potentially even further reducing bundle size for things that also exist in @expressive-core/core that may not be fully required for EC to function properly. I haven't gone down the entire path yet of what is/isn't needed for baseline functionality, but this could further refine @expressive-code/core to truly be a Core layer and only contain the bare minimum.
Lemme know your thoughts, thanks!
2 participants
As discussed in #326, other than for
astro-expressive-code, there is no current way to control the bundle size when usingexpressive-codeoutside of somepackage.jsonrelated workarounds. This PR aims to address that by providing stock support for controlling bundle size by introducing a newCoreconcept across the various packages (e.g.,ExpressiveCodeCore,RehypeExpressiveCodeCore, etc.).Note
The approach taken in this PR is to ensure 100% backwards compatibility with
v0.41.2. All existing tests pass without any modification. If it's acceptable to introduce some minor adjustments, the current approach can be simplified in some areas. Sinceexpressive-codeuses semver0.x.x, technically every release is a breaking change so for the purposes of long-term maintenance, some changes may be worth considering.TLDR;
shiki=falsein configuration makes no difference to final bundle size when usingExpressiveCode/rehypeExpressiveCode/createRenderer. To reduce bundle size, you must useCoredirectly.renderedLengthof corresponding item, theFull Bundlecolumn is the actual final size of the module on diskThe reasonPluginFramesandPluginTextMarkersare not zero inCoreis primarily due to styles (css) which could likely be refactored outAdditional Information
remark-expressive-codeappears to be deprecated so it was not updated forCoreastro-expressive-codeneeds to be updated toCore. Once a decision is made on potentially incorporating this PR and theCoreAPI finalized, I'll look at updating it to useCorerather than itsvite-pluginapproach.RehypeExpressiveCodeandRehypeExpressiveCodeCore. Indistfolder of each fixture are statistics files that can be reviewed for detailed information about the build output. See visualizer and bundle-stats for more information.Open to any and all feedback, appreciate your consideration!
Resolves #326