|
| 1 | +# Architecture Improvements |
| 2 | + |
| 3 | +Tracked improvements for react-shiki codebase. |
| 4 | + |
| 5 | +## High Priority |
| 6 | + |
| 7 | +### Consolidate Bundle Factories |
| 8 | +`bundles/full.ts` and `bundles/web.ts` are nearly identical - only the import differs. Extract shared logic into a factory that accepts bundle configuration. |
| 9 | + |
| 10 | +```typescript |
| 11 | +// Before: Two nearly identical files |
| 12 | +// bundles/full.ts imports from 'shiki' |
| 13 | +// bundles/web.ts imports from 'shiki/bundle/web' |
| 14 | + |
| 15 | +// After: Single factory with config |
| 16 | +const createBundleHighlighter = (getSingletonHighlighter, createEngine) => |
| 17 | + async (langs, themes, engine) => { ... } |
| 18 | +``` |
| 19 | + |
| 20 | +### Consolidate Entry Points |
| 21 | +`index.ts`, `web.ts`, and `core.ts` repeat the same wrapper pattern. Create a shared entry point factory. |
| 22 | + |
| 23 | +```typescript |
| 24 | +// Current: Same pattern repeated 3 times |
| 25 | +export const useShikiHighlighter = <F extends OutputFormat = 'react'>(...) => { |
| 26 | + return useBaseHook(..., createXxxHighlighter); |
| 27 | +}; |
| 28 | +export const ShikiHighlighter = createShikiHighlighterComponent(useShikiHighlighter); |
| 29 | + |
| 30 | +// Proposed: Entry point factory |
| 31 | +const { useShikiHighlighter, ShikiHighlighter } = createEntryPoint(createFullHighlighter); |
| 32 | +``` |
| 33 | +
|
| 34 | +## Medium Priority |
| 35 | +
|
| 36 | +### Type-Safe Transformer Registry |
| 37 | +The output transformer registry loses type information on lookup: |
| 38 | +```typescript |
| 39 | +return outputTransformers[format](context) as OutputFormatMap[F]; |
| 40 | +``` |
| 41 | +
|
| 42 | +Could use conditional types or function overloads for full type safety. |
| 43 | +
|
| 44 | +### Centralize Constants |
| 45 | +`DEFAULT_THEMES` is defined in `options.ts`. Consider a `constants.ts` for all defaults: |
| 46 | +- Default themes |
| 47 | +- Default line number start |
| 48 | +- Default output format |
| 49 | +
|
| 50 | +### Split Types File |
| 51 | +`types.ts` mixes public API types with internal types. Consider: |
| 52 | +- `types.ts` - Public API types (Language, Theme, HighlighterOptions) |
| 53 | +- `internal-types.ts` - Internal types (TimeoutState, TransformContext) |
| 54 | +
|
| 55 | +## Low Priority |
| 56 | +
|
| 57 | +### Surface Errors to Consumers |
| 58 | +Currently errors are logged but not surfaced: |
| 59 | +```typescript |
| 60 | +highlight().catch(console.error); |
| 61 | +``` |
| 62 | +
|
| 63 | +Consider an `onError` callback option or returning error state from hook. |
| 64 | +
|
| 65 | +### Component Output Type |
| 66 | +The component casts the hook return: |
| 67 | +```typescript |
| 68 | +) as ReactNode | string | null; |
| 69 | +``` |
| 70 | +
|
| 71 | +This is because the component restricts `outputFormat` to `'react' | 'html'` but still uses the generic hook. Could create a component-specific hook variant. |
| 72 | +
|
| 73 | +### Empty String Fallbacks |
| 74 | +Token output uses empty strings as fallbacks: |
| 75 | +```typescript |
| 76 | +fg: theme?.fg ?? '', |
| 77 | +bg: theme?.bg ?? '', |
| 78 | +``` |
| 79 | +
|
| 80 | +Consider `undefined` for missing values, letting consumers decide on fallbacks. |
0 commit comments