fix: make gitlabPlugin conform to Docusaurus PluginModule - #55
Merged
Merged
Conversation
The default export was not assignable to `PluginModule`, and its return
value was not a valid `Plugin`, so the registration form documented in
README.md and examples/gitlab — `plugins: [[gitlabPlugin, opts]]` — failed
to type-check in any TypeScript Docusaurus site:
error TS2322: Type '[(context: unknown, options: PluginOptions) => ...]'
is not assignable to type 'PluginConfig'.
Two causes:
1. `mergeStrategy: { "module.rules": "append" }` inferred the literal as
`string`, which is not a webpack-merge `CustomizeRuleString`, so the
configureWebpack result violated `ConfigureWebpackResult`. The runtime
value was always correct; only the type was wrong. It went unnoticed
because the factory has no return-type annotation, so the shape was
inferred and never compared against `Plugin`.
2. `options: PluginOptions` is narrower than `PluginModule`'s
`options: unknown` and fails contravariantly. Docusaurus's own plugins
(e.g. @docusaurus/plugin-sitemap) share this and are likewise not
assignable; widening both parameters holds a stricter line than
first-party so the documented function form type-checks.
Neither was detectable before: @docusaurus/types was not a dependency at
any level, and neither example site had a tsconfig.json, so `typecheck`
could not see the plugin contract at all.
Add two guards:
- src/plugin/types.test.ts asserts assignability to `PluginModule`,
`Plugin`, and `PluginConfig`. tsconfig.build.json excludes *.test.ts, so
the @docusaurus/types import never reaches dist/ — a .d.ts importing it
would fail to resolve for pnpm consumers, who do not get the package
hoisted (examples/gitlab has to declare it explicitly).
- examples/gitlab gains a tsconfig.json and a `typecheck` script, so the
consumer-side registration is checked against the real `Config` type.
Scoped to docusaurus.config.ts and sidebars.ts: src/theme/* swizzles
import `@theme-original/*`, a webpack alias that only resolves against a
generated .docusaurus/ dir.
Verified both guards fail when the fix is reverted, the example one
pointing at the documented line in docusaurus.config.ts.
Three reviewers (impact, security, code style) converged on the same set of
issues. All verified before and after.
Test pollution (security + style, PR-introduced): types.test.ts called the
factory without an assetDir, so `resolveOptions` defaulted it to the relative
"static/gitlab-assets" and the eager `ctx.assets.sync()` created that directory
in the repo root on every `pnpm test`. .gitignore:23 only covers
examples/*/static/. Confirmed by bisection: the full suite with this file
removed creates nothing; the file alone creates it. Both sibling tests avoid
this deliberately and say so (index.test.ts:15, packaging.test.ts).
The runtime `it()` could not catch the bug it claimed to. Types are erased, so
removing `as const` left the assertion passing, and it duplicated
index.test.ts:69, which asserts the same thing with the webpack-merge rationale.
Dropped it. vitest.config.ts sets passWithNoTests, so a pure type-assertion file
is a clean pass — which removes the assetDir problem entirely rather than
patching it.
Switched to `satisfies`, the idiom this repo already uses (8× in
gitlab/fetchers.ts); `void _x` / `const _x` placeholders had no precedent
anywhere in src/ or test/, and eslint's varsIgnorePattern "^_" made the `void`
statements redundant regardless. The separate `_returnsPlugin` assertion was
subsumed by the module-level one, which already pins the return type. Verified
the two remaining lines still catch both regressions independently: reverting
`as const` and re-narrowing the options parameter each produce 2 errors.
Renamed `rawOptions` back to `options` with a `pluginOptions` local, matching
the `context`/`loadContext` pattern one line below. The parameter name is
public API surface — it shows in dist/plugin/index.d.ts and in consumers'
editor tooltips.
Wired the consumer-side guard into CI and mise. It was documentation before:
ci.yml ran only the root typecheck, whose tsconfig includes just src and test,
and mise.toml had no task. The new CI step goes in the `test` job, which
already builds dist/ — the example's workspace link resolves to it, so the
`lint` job could not run this.
Trimmed the contravariance explanation, which had been written three times in
near-identical words; CLAUDE.md keeps the canonical copy. Documented instead
that `?? {}` is load-bearing: without it `resolveOptions(undefined)` passes
Joi's object schema and dies on `opts.host` with a raw TypeError rather than
the branded "host is required".
Gate: build, typecheck, example typecheck, lint (0 errors), 677 tests across
59 files, no stray static/, and dist/ still free of @docusaurus/types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The default export is not assignable to Docusaurus's
PluginModule, and its return value is not a validPlugin. As a result the registration form documented inREADME.mdand used byexamples/gitlab— the function form, which is what Docusaurus actually type-checks againstPluginConfig— fails to compile in any TypeScript Docusaurus site:Two causes
1.
mergeStrategywidened tostring.mergeStrategy: { "module.rules": "append" }inferred the literal asstring, which is not a webpack-mergeCustomizeRuleString, so theconfigureWebpackresult violatedConfigureWebpackResult. The runtime value was always correct — only the type was wrong. It went unnoticed because the factory has no return-type annotation, so its shape was inferred and never compared againstPlugin. Docusaurus's own plugins annotatePromise<Plugin<Content>>explicitly and would have caught this.2.
options: PluginOptionsfails contravariantly againstPluginModule'soptions: unknown. Worth noting this one is not specific to this package — I checked@docusaurus/plugin-sitemap, declared(context: LoadContext, options: PluginOptions), and it fails identically. Widening both parameters holds a stricter line than first-party so the documented form type-checks.context: unknownwas already fine —unknownis a supertype ofLoadContext, so it passes contravariantly. The hand-rolledCliLikeshim is also structurally compatible with Commander'sCommanderStatic.Why nothing caught it
@docusaurus/typeswas not a dependency at any level — not dep, devDep, or peer — and it is not even resolvable fromexamples/gitlabunder pnpm. Neither example site had atsconfig.json, so theconst config: Configannotation indocusaurus.config.tswas never checked.pnpm run typecheckstructurally could not see the plugin contract.The fix
Two lines in
src/plugin/index.ts, each commented as load-bearing, plus two guards:src/plugin/types.test.ts— asserts assignability toPluginModule,Plugin, andPluginConfig.tsconfig.build.jsonexcludes*.test.ts, so the@docusaurus/typesimport never reachesdist/. This matters: a.d.tsimporting it would fail to resolve for pnpm consumers, who do not get the package hoisted —examples/gitlabhas to declare it explicitly, which is the proof.examples/gitlab/tsconfig.json+ atypecheckscript — the consumer-side guard, checking the real registration against the realConfigtype. Scoped todocusaurus.config.tsandsidebars.ts:src/theme/*swizzles import@theme-original/*, a webpack alias that only resolves against a generated.docusaurus/dir, so including it would demand a full site build just to type-check.Verification
Both guards were written first and confirmed failing, then passing. I also verified they actually bite rather than passing vacuously — reverting only the
as constand rebuilding fails both, the example one pointing straight at the documented line:With the fix in place:
typecheckclean (root andexamples/gitlab)future.v4variantslint0 errors, markdownlint 0 issuesgrep -rn "@docusaurus/types" dist/returns nothing — no devDependency leak into the published packageNote for reviewers
This changes the exported signature to
(context: unknown, options: unknown). It is source-compatible for every documented usage — the string form, the function form, and direct calls are all unaffected (widening a parameter is assignment-safe), and the narrowing back toPluginOptionshappens on the first line whereresolveOptionsvalidates with Joi regardless.An earlier version of this note claimed callers lose option checking on the second argument. That was wrong — they never had it. Docusaurus's tuple slot is its own
PluginOptions = {id?: string} & {[key: string]: unknown}, an index signature that accepts anything, so[[gitlabPlugin, { strcit: true }]]compiled clean before this PR too. An overload preserving the narrow signature was tested and rejected: a typo'd literal silently falls through to the wide overload. What does give consumers real checking issatisfies PluginOptionson the options object in the README snippet andexamples/gitlab— worth a follow-up.Review round
Reviewed from three angles (impact, security, code style). Fixes applied in 3664021:
it()could not catch its own bug. Types are erased, so removingas constleft it passing, and it duplicatedindex.test.ts:69. Replaced the whole file with twosatisfieslines — the idiom this repo already uses 8× ingitlab/fetchers.ts. Verified both regressions are still caught independently.static/gitlab-assets/in the root on everypnpm test(noassetDir, so the eagerassets.sync()used the relative default). Confirmed by bisection. Dropping the runtime half removed the cause rather than patching it.ci.ymlonly ran the root typecheck, whose tsconfig covers justsrcandtest, andmise.tomlhad no task. Now wired into both; the CI step sits in thetestjob, which already buildsdist/.rawOptionsrenamed back tooptions; the parameter name is public API surface visible indist/plugin/index.d.ts.?? {}is load-bearing (it converts a rawTypeErroronopts.hostinto the branded validation error).Out of scope, raised separately: the security review surfaced a pre-existing HIGH finding unrelated to this PR — plugin options are serialized verbatim into the client bundle via Docusaurus's
siteConfig, so a configuredtokenships to the browser. Independently reproduced. Not addressed here.