Conversation
created module augmentation in .d.ts files for components patches, with types inferred by studying the usage in the node_modules src files. added tsc compiled .js files to .gitignore and prevent .d.ts files from being ignored Ignore compiled js and patched ts from linting
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2853 +/- ##
==========================================
- Coverage 71.92% 71.89% -0.03%
==========================================
Files 132 132
Lines 7358 7359 +1
Branches 1516 1637 +121
==========================================
- Hits 5292 5291 -1
- Misses 2021 2062 +41
+ Partials 45 6 -39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates MarkBind’s internal “core patches” (monkey-patches applied to nunjucks, htmlparser2, and a markdown-it custom-component patch set) from CommonJS .js files to TypeScript sources, and adds local .d.ts files to type internal/unstable APIs that aren’t covered by upstream @types/* packages.
Changes:
- Converted patch entrypoints from
*.jsto*.tsand updated exports/imports accordingly. - Added manual TypeScript declaration files for patched/hidden internals (
nunjuckssubmodules + internals,htmlparser2internals). - Updated ignore lists to avoid committing compiled patch outputs while still tracking the “manual” declaration files.
Reviewed changes
Copilot reviewed 13 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/patches/nunjucks/nunjucks-submodules.d.ts | Adds ambient module declarations for untyped nunjucks internal submodules. |
| packages/core/src/patches/nunjucks/nunjucks-internals.d.ts | Adds module augmentations for nunjucks internals used by MarkBind patches. |
| packages/core/src/patches/nunjucks/load-event.ts | Migrates the nunjucks “load event” patch to TS + ES imports. |
| packages/core/src/patches/nunjucks/index.ts | TS patch entrypoint (side-effect imports). |
| packages/core/src/patches/nunjucks/index.js | Removes old CommonJS entrypoint. |
| packages/core/src/patches/nunjucks/context-overrides-frame.ts | Migrates and types the MarkBind context/frame priority patch. |
| packages/core/src/patches/index.ts | Migrates patch aggregator to TS and wires ignore-tag injections. |
| packages/core/src/patches/index.js | Removes old CommonJS patch aggregator. |
| packages/core/src/patches/htmlparser2.ts | Migrates htmlparser2 patch to TS + ES imports and exports. |
| packages/core/src/patches/htmlparser2-internal.d.ts | Adds declaration augmentation for htmlparser2 internals used by the patch. |
| packages/core/src/lib/markdown-it/patches/custom-component/inlineTags.ts | Converts export style to TS-friendly export =. |
| packages/core/src/lib/markdown-it/patches/custom-component/htmlRe.ts | Converts to named ESM exports. |
| packages/core/src/lib/markdown-it/patches/custom-component/htmlInlineRule.ts | Migrates to TS imports/exports and adds types. |
| packages/core/src/lib/markdown-it/patches/custom-component/htmlBlockRule.ts | Migrates to TS imports/exports and adds types. |
| packages/core/src/lib/markdown-it/patches/custom-component/customComponentPlugin.ts | Migrates plugin wiring to TS imports/exports and adds types. |
| .gitignore | Ignores compiled patch JS outputs; attempts to unignore manual .d.ts files. |
| .eslintignore | Ignores compiled patch JS outputs for linting. |
Comments suppressed due to low confidence (3)
packages/core/src/patches/htmlparser2.ts:465
injectIgnoreTagsis typed asstring[], but the caller path (PluginManager->patches.ignoreTags) passes aSetof tags. Since the implementation only spreads the input, it can support any iterable; consider changing the parameter type toIterable<string>(orreadonly string[]) and converting to an array before spreading if needed.
packages/core/src/lib/markdown-it/patches/custom-component/customComponentPlugin.ts:41injectTagsis typed astagsToIgnore: string[], but the upstream call path (PluginManager->patches.ignoreTags) passes aSetof tags. Consider widening this (and thecustomComponentPluginparameter) toIterable<string>/ReadonlySet<string> | readonly string[]so the existing call-site type-checks without forcing an array conversion.
packages/core/src/lib/markdown-it/patches/custom-component/htmlBlockRule.ts:6initCustomComponentHtmlBlockRuleis typed to takestring[], but the surrounding logic (Array.from(tagsToIgnore)) works with any iterable and the upstream caller provides aSetof tags. Widening this parameter toIterable<string>(or acceptingSet<string> | string[]) would align with actual usage and avoid type errors once the caller is updated.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { injectTags } from '../lib/markdown-it/patches/custom-component/customComponentPlugin'; | ||
| import { injectIgnoreTags } from './htmlparser2'; | ||
|
|
||
| function ignoreTags(tagsToIgnore: string[]) { |
There was a problem hiding this comment.
ignoreTags is typed to accept string[], but its only caller (PluginManager._collectPluginTagConfigs) passes a Set of tag names. This will fail type-checking and also forces downstream helpers (injectIgnoreTags / injectTags) to accept only arrays even though they work fine with iterables. Consider widening the parameter type to Iterable<string> (or Set<string> | string[]) and updating the injected functions’ signatures accordingly.
| function ignoreTags(tagsToIgnore: string[]) { | |
| function ignoreTags(tagsToIgnore: Iterable<string>) { |
There was a problem hiding this comment.
Yes, the typing for this seems to be inaccurate?
Is it typecasting or something?
yihao03
force-pushed
the
feat/migrate-core-patch
branch
from
0aa1499 to
ea62ea3
Compare
gerteck
left a comment
gerteck
left a comment
There was a problem hiding this comment.
Highlighted some parts of the code that needs a further look and update, but generally looks good
|
|
||
| const initCustomComponentHtmlBlockRule = require('./htmlBlockRule'); | ||
| const htmlInlineRule = require('./htmlInlineRule'); | ||
| import MarkdownIt from 'markdown-it'; |
There was a problem hiding this comment.
Prefer using pure type imports
import type MarkdownIt from 'markdown-it';
| @@ -1,13 +1,14 @@ | |||
| const { HTML_TAG_RE } = require('./htmlRe'); | |||
| import StateInline from 'markdown-it/lib/rules_inline/state_inline'; | |||
There was a problem hiding this comment.
prefer using import type here as well
| // Frame and contextOrFrameLookup. ESM namespace imports (import * as ...) are | ||
| // frozen objects, so we fall back to require() which returns the original mutable exports. | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const globalRuntime: typeof runtimeTypes & Record<string, any> = require('nunjucks/src/runtime'); |
There was a problem hiding this comment.
This seems like a future blocker:
This Nunjucks patch represents a critical but fragile architectural choice in MarkBind, specifically designed to override Nunjucks' default variable scoping so that variables passed via take priority over internal {% set %} declarations.
- To achieve this, the code performs "monkey-patching" by manually overwriting the prototypes of internal, non-exported classes like Frame and Context. However, because modern TypeScript and ESM (EcmaScript Modules) treat imports as "frozen" read-only namespaces that cannot be modified, the implementation employs a "TypeScript hack" by using require('nunjucks/src/runtime') to obtain a mutable CommonJS object while using import type for development-time safety.
- While this approach is functional in a CommonJS environment, it creates significant technical debt and follows a fragile pattern that relies on private library internals; shifting to a Pure ESM output in the future will break this mechanism entirely, as require will be unavailable and the ESM standard is explicitly designed to prevent this type of global prototype surgery.
Need to consider and think carefully how we should work on this
There was a problem hiding this comment.
I am not sure on the part about require() being not available in typescript, see this. require can be available by importing from node:module, though it means that it wouldn’t work on browser runtime. however afaik the nunjucks code won’t be present in browser runtime?
alternatively, i have considered just cloning nunjucks to be part of this repo, considering its last release was 2 years ago meaning that it’s unlikely to get new features that we might need.
| // --------------------------------------------------------------------------- | ||
| // Sub-module: nunjucks/src/runtime | ||
| // --------------------------------------------------------------------------- | ||
| declare module 'nunjucks/src/runtime' { |
There was a problem hiding this comment.
Duplicated nunjucks/src/runtime and nunjucks/src/object declarations in
internals.d.tsandsubmodule.d.ts
Any reason why it was duplicated? Should it be removed from internals.d.ts?
There was a problem hiding this comment.
nunjucks-submodules.d.tsexists solely to create the sub-modules (nunjucks/src/runtime, nunjucks/src/object) as ambient declarations.nunjucks-internals.d.tsexists to augment the main nunjucks module, big declare module 'nunjucks' block at the bottom is its unique, non-duplicated work.
The sub-module blocks in it are dead weight that snuck in.
So the rule is: whoever creates something owns it. nunjucks-submodules.d.ts created the sub-modules, so it should keep the duplicated code. Delete the duplicate sub-module blocks from nunjucks-internals.d.ts.
| import { injectTags } from '../lib/markdown-it/patches/custom-component/customComponentPlugin'; | ||
| import { injectIgnoreTags } from './htmlparser2'; | ||
|
|
||
| function ignoreTags(tagsToIgnore: string[]) { |
There was a problem hiding this comment.
Yes, the typing for this seems to be inaccurate?
Is it typecasting or something?
What is the purpose of this pull request?
Overview of changes:
Migrated
nunjucks,htmlparser2andmarkdown-itpatches to TypeScript and ESM.Anything you'd like to highlight/discuss:
As these patches are modifying the internals of the npm packages, the types
we worked on are not exposed in the
@typespackages. We have to do module augmentationto add the types that we are patching. Considering that the packages haven't been updated
in a while and will likely not be updated in the near future, we are fine with this approach.
Testing instructions:
Pull this branch and run
npm run setupto make sure exisitng .js files are cleanedand .ts files are compiled. Then run
npm testto make sure all tests are passing.Proposed commit message: (wrap lines at 72 characters)
Migrated
nunjucks,htmlparser2andmarkdown-itpatches to TypeScript and ESM.Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):