fix(types): fix circular deps#1099
Merged
makhnatkin merged 2 commits intomainfrom Apr 24, 2026
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
|
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
makhnatkin
force-pushed
the
fix/types-circular-deps
branch
from
172ce6e to
efdf942
Compare
makhnatkin
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
makhnatkin
force-pushed
the
fix/types-circular-deps
branch
from
20eacd2 to
59fa46f
Compare
makhnatkin
force-pushed
the
fix/types-circular-deps
branch
from
5b81eb9 to
b036c83
Compare
Reviewer's GuideRefactors the editor package’s type structure to eliminate type-only circular dependencies while keeping the external public API stable, and tightens the circular-deps CI check to be type-aware by disabling dpdm’s transform step. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The move of
Extension*types fromcore/types/extensionintoExtensionBuilderis good for breaking cycles, butcore/index.tsno longer re-exports these types at all; ifExtension/ExtensionAuto/etc. were part of the publiccoreAPI, consider re-exporting them fromcore/index.tsviaExtensionBuilderto avoid a breaking change. - With
MarkdownSerializerDynamicModifiernow co-located inMarkdownSerializer.ts, the public entry pointcore/markdown/MarkdownSerializerDynamicModifier.tsis reduced to a thin re-export; it might be worth adding a brief comment there documenting that this file is a compatibility facade so future refactors don’t accidentally remove it and break deep imports.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The move of `Extension*` types from `core/types/extension` into `ExtensionBuilder` is good for breaking cycles, but `core/index.ts` no longer re-exports these types at all; if `Extension`/`ExtensionAuto`/etc. were part of the public `core` API, consider re-exporting them from `core/index.ts` via `ExtensionBuilder` to avoid a breaking change.
- With `MarkdownSerializerDynamicModifier` now co-located in `MarkdownSerializer.ts`, the public entry point `core/markdown/MarkdownSerializerDynamicModifier.ts` is reduced to a thin re-export; it might be worth adding a brief comment there documenting that this file is a compatibility facade so future refactors don’t accidentally remove it and break deep imports.
## Individual Comments
### Comment 1
<location path="packages/editor/src/core/ExtensionBuilder.ts" line_range="299" />
<code_context>
return map;
}
+export type Extension = (builder: ExtensionBuilder) => void;
+export type ExtensionWithOptions<T> = (builder: ExtensionBuilder, options: T) => void;
+export type ExtensionAuto<T = void> = T extends void ? Extension : ExtensionWithOptions<T>;
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving this new block of extension-related type definitions into a dedicated, colocated type module so the main builder file stays focused on behavior.
You can reduce cognitive load here by factoring the new type block back into a dedicated type module colocated with `ExtensionBuilder`, while keeping all behavior unchanged.
For example, extract the type definitions into a separate file:
```ts
// ExtensionTypes.ts (or ./types/extension.ts if that’s the established pattern)
import type MarkdownIt from 'markdown-it';
import OrderedMap from 'orderedmap';
import type {MarkSpec, NodeSpec, Schema} from 'prosemirror-model';
import type {ActionSpec, ActionStorage} from './types/actions';
import type {MarkViewConstructor, NodeViewConstructor} from './types/node-views';
import type {Parser, ParserToken} from './types/parser';
import type {Serializer, SerializerMarkToken, SerializerNodeToken} from './types/serializer';
import type {ExtensionBuilder} from './ExtensionBuilder';
export type Extension = (builder: ExtensionBuilder) => void;
export type ExtensionWithOptions<T> = (builder: ExtensionBuilder, options: T) => void;
export type ExtensionAuto<T = void> = T extends void ? Extension : ExtensionWithOptions<T>;
export type ExtensionNodeSpec = {
spec: NodeSpec;
view?: (deps: ExtensionDeps) => NodeViewConstructor;
fromMd: {
tokenName?: string;
tokenSpec: ParserToken;
};
toMd: SerializerNodeToken;
};
export type ExtensionMarkSpec = {
spec: MarkSpec;
view?: (deps: ExtensionDeps) => MarkViewConstructor;
fromMd: {
tokenName?: string;
tokenSpec: ParserToken;
};
toMd: SerializerMarkToken;
};
export type ExtensionDeps = {
readonly schema: Schema;
readonly textParser: Parser;
readonly markupParser: Parser;
readonly serializer: Serializer;
readonly actions: ActionStorage;
};
export type ExtensionSpec = {
configureMd(md: MarkdownIt, parserType: 'text' | 'markup'): MarkdownIt;
nodes(): OrderedMap<ExtensionNodeSpec>;
marks(): OrderedMap<ExtensionMarkSpec>;
plugins(deps: ExtensionDeps): Plugin[];
actions(deps: ExtensionDeps): Record<string, ActionSpec>;
};
```
Then keep `ExtensionBuilder` focused on behavior:
```ts
// ExtensionBuilder.ts
import type {Extension, ExtensionAuto, ExtensionSpec, ExtensionDeps} from './ExtensionTypes';
// ...rest of imports
export class ExtensionBuilder {
// existing implementation unchanged
}
```
This preserves all the new capabilities (`Schema`, `Parser`, `Serializer`, `ActionStorage`, view constructors, etc.) while:
- Making `ExtensionBuilder.ts` easier to scan (behavior first, types elsewhere).
- Centralizing the extension type system in one reusable module, reducing the risk of drift with `core/types/extension.ts`.
- Keeping the large type-level surface area in a dedicated place that can evolve independently of the builder logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
d3m1d0v
reviewed
Apr 23, 2026
- Break type-only circular dependencies by introducing leaf type modules and co-locating option types with their schema implementations. - WidgetDecoration: extract `Meta` to leaf module `meta.ts` (parameterized by descriptor type to avoid the type-only cycle); restore `satisfies Meta` in `actions.ts` and `WidgetDescriptor#applyTo`. Public `Meta` in `types.ts` is exposed as `Meta = MetaGeneric<WidgetDescriptor>`. - core/index.ts: keep explicit re-export of `Extension*` types from `./ExtensionBuilder` for an explicit public API surface. - YfmNoteSpecs/DeflistSpecs: move schema-level option type into `schema.ts` as `*SchemaOptions`; keep public `*SpecsOptions` in `index.ts` as an alias of the schema options (matches the existing Tabs/Table/Cut/Checkbox convention). - Configure check-circular-deps to surface type-only cycles (no longer transformed away). Made-with: Cursor
makhnatkin
force-pushed
the
fix/types-circular-deps
branch
from
b036c83 to
619f5a5
Compare
d3m1d0v
approved these changes
Apr 24, 2026
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
2 participants
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.
Summary
Fixes type-only circular imports that were invisible in CI because
scripts/check-circular-deps.jsrandpdmwithtransform: true, strippingimport typeedges. Drops that flag and refactors the dependency graph so the cycle threshold can be lowered to0with types.Changes
scripts/check-circular-deps.jsis now type-aware (transform: false).bundle/events,bundle/preset-base-types,bundle/editor-public-types,GPT/types,html-to-markdown/types), co-located extension/serializer types with their implementations (ExtensionBuilder.ts,MarkdownSerializer.ts), and replaced barrel imports with direct deep imports on the remaining hot paths.core/types/extension.ts,bundle/types.ts,bundle/Editor.tsandcore/markdown/MarkdownSerializerDynamicModifier.tskeep their public shapes as thin re-export facades, so@gravity-ui/markdown-editor/_/*deep imports stay green.Summary by Sourcery
Make the circular dependency check type-aware and reorganize editor type definitions into dedicated modules to avoid type-only import cycles while preserving the existing public API.
Bug Fixes:
Enhancements: