Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: fixed critical circular dependencies #549

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: fixed critical circular dependencies #549

wants to merge 6 commits into from

Conversation

obenjiro
Copy link
Collaborator

@obenjiro obenjiro commented Jan 16, 2025 •
edited by makhnatkin
Loading

I found one unpleasant issue. Currently, when updating to the new version 14.10.2, everything falls apart a bit.

The main problem is that in some cases at runtime i get this error:
Top-level error ReferenceError: Cannot access 'WToolbarTextSelect' before initialization

Such problems can occur due to Circular Dependencies (there's a pretty good description of the problem here: metaplex-foundation/solita#22)

In short, the problem has always been there. You can check it by running npx dpdm --no-warning --no-tree ./src/index.ts from root folder of this repo. Right now there is 107 circular dependencies in the project

Here is a circular dependencies that need to be solved:

1) src/toolbar/index.ts -> src/bundle/config/index.ts -> src/bundle/config/wysiwyg.ts -> src/extensions/index.ts -> src/extensions/behavior/index.ts -> src/extensions/behavior/CommandMenu/index.ts -> src/extensions/markdown/index.ts -> src/extensions/markdown/Link/index.ts -> src/extensions/markdown/Link/paste-plugin.ts -> src/extensions/markdown/Image/index.ts -> src/extensions/markdown/Image/imageUrlPaste/index.ts -> src/bundle/index.ts -> src/bundle/MarkdownEditorView.tsx -> src/bundle/settings/index.tsx


2) src/toolbar/index.ts -> src/bundle/config/index.ts -> src/bundle/config/wysiwyg.ts -> src/extensions/index.ts -> src/extensions/behavior/index.ts -> src/extensions/behavior/CommandMenu/index.ts -> src/extensions/markdown/index.ts -> src/extensions/markdown/Link/index.ts -> src/extensions/markdown/Link/paste-plugin.ts -> src/extensions/markdown/Image/index.ts -> src/extensions/markdown/Image/imageUrlPaste/index.ts -> src/bundle/index.ts -> src/bundle/MarkdownEditorView.tsx -> src/bundle/toolbar/utils/toolbarsConfigs.ts -> src/modules/toolbars/presets.ts -> src/modules/toolbars/items.tsx -> src/bundle/toolbar/wysiwyg/WToolbarColors.tsx


3) src/toolbar/index.ts -> src/bundle/config/index.ts -> src/bundle/config/wysiwyg.ts -> src/extensions/index.ts -> src/extensions/behavior/index.ts -> src/extensions/behavior/CommandMenu/index.ts -> src/extensions/markdown/index.ts -> src/extensions/markdown/Link/index.ts -> src/extensions/markdown/Link/paste-plugin.ts -> src/extensions/markdown/Image/index.ts -> src/extensions/markdown/Image/imageUrlPaste/index.ts -> src/bundle/index.ts -> src/bundle/MarkdownEditorView.tsx -> src/bundle/toolbar/utils/toolbarsConfigs.ts -> src/modules/toolbars/presets.ts -> src/modules/toolbars/items.tsx -> src/bundle/toolbar/wysiwyg/WToolbarTextSelect.tsx


4) src/bundle/config/wysiwyg.ts -> src/extensions/index.ts -> src/extensions/behavior/index.ts -> src/extensions/behavior/CommandMenu/index.ts -> src/extensions/markdown/index.ts -> src/extensions/markdown/Link/index.ts -> src/extensions/markdown/Link/paste-plugin.ts -> src/extensions/markdown/Image/index.ts -> src/extensions/markdown/Image/imageUrlPaste/index.ts -> src/bundle/index.ts -> src/bundle/MarkdownEditorView.tsx -> src/bundle/toolbar/utils/toolbarsConfigs.ts -> src/modules/toolbars/presets.ts -> src/modules/toolbars/items.tsx -> src/bundle/toolbar/wysiwyg/WToolbarTextSelect.tsx


5) src/toolbar/index.ts -> src/bundle/config/index.ts -> src/bundle/config/wysiwyg.ts -> src/extensions/index.ts -> src/extensions/behavior/index.ts -> src/extensions/behavior/CommandMenu/index.ts -> src/extensions/markdown/index.ts -> src/extensions/markdown/Link/index.ts -> src/extensions/markdown/Link/paste-plugin.ts -> src/extensions/markdown/Image/index.ts -> src/extensions/markdown/Image/imageUrlPaste/index.ts -> src/bundle/index.ts -> src/bundle/MarkdownEditorView.tsx -> src/bundle/toolbar/utils/toolbarsConfigs.ts -> src/modules/toolbars/presets.ts -> src/modules/toolbars/items.tsx -> src/bundle/toolbar/wysiwyg/WToolbarTextSelect.tsx -> src/bundle/toolbar/ToolbarSelect.tsx


6) src/bundle/config/wysiwyg.ts -> src/extensions/index.ts -> src/extensions/behavior/index.ts -> src/extensions/behavior/CommandMenu/index.ts -> src/extensions/markdown/index.ts -> src/extensions/markdown/Link/index.ts -> src/extensions/markdown/Link/paste-plugin.ts -> src/extensions/markdown/Image/index.ts -> src/extensions/markdown/Image/imageUrlPaste/index.ts -> src/bundle/index.ts -> src/bundle/MarkdownEditorView.tsx -> src/bundle/toolbar/utils/toolbarsConfigs.ts -> src/modules/toolbars/presets.ts -> src/modules/toolbars/items.tsx -> src/bundle/toolbar/wysiwyg/WToolbarTextSelect.tsx -> src/bundle/toolbar/ToolbarSelect.tsx

Here what i did:

Moved Types to a Central Location:

Created a new file src/bundle/toolbar/types.ts that now serves as the central location for all toolbar-related types
Moved all toolbar types from src/toolbar/types.ts to this new location
Made src/toolbar/types.ts re-export everything from the new location (to avoid major release)

Separated Heading Configuration:

Created a new file src/bundle/config/w-heading-config.ts to separate heading-related configurations
This helped break up circular dependencies by moving specific toolbar configurations to their own files

Added Circular Dependency Detection:

Added dpdm as a dev dependency in package.json
Added a new npm script "circular-deps": "dpdm --no-warning --no-tree ./src/index.ts" to help detect circular dependencies

Right now there is only 99 circular dependencies left :)

@d3m1d0v
Copy link
Member

d3m1d0v commented Jan 16, 2025

Very cool! Thanks!

@d3m1d0v
Copy link
Member

d3m1d0v commented Jan 16, 2025

Added Circular Dependency Detection:

Added dpdm as a dev dependency in package.json Added a new npm script "circular-deps": "dpdm --no-warning --no-tree ./src/index.ts" to help detect circular dependencies

Do u have any idea of adding this check to ci-checks in PRs?

That should cause an error when number of cyclic dependencies has been increased...

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@obenjiro
Copy link
Collaborator Author

Added Circular Dependency Detection:

Added dpdm as a dev dependency in package.json Added a new npm script "circular-deps": "dpdm --no-warning --no-tree ./src/index.ts" to help detect circular dependencies

Do u have any idea of adding this check to ci-checks in PRs?

That should cause an error when number of cyclic dependencies has been increased...

Added CI check called "Check Circular Dependencies (pull_request) "

obenjiro
obenjiro commented Jan 16, 2025
View reviewed changes
package.json Outdated Show resolved Hide resolved
d3m1d0v
d3m1d0v reviewed Jan 16, 2025
View reviewed changes
src/toolbar/types.ts Outdated Show resolved Hide resolved
src/toolbar/types.ts Outdated Show resolved Hide resolved
src/toolbar/flexible.tsx Outdated Show resolved Hide resolved
src/toolbar/FlexToolbar.tsx Outdated Show resolved Hide resolved
src/extensions/behavior/SelectionContext/tooltip.tsx Outdated Show resolved Hide resolved
src/bundle/toolbar/wysiwyg/WToolbarTextSelect.tsx Outdated Show resolved Hide resolved
d3m1d0v
d3m1d0v reviewed Jan 17, 2025
View reviewed changes
src/toolbar/flexible.tsx Outdated Show resolved Hide resolved
@d3m1d0v d3m1d0v changed the title fix(core): fix critical circular dependencies (Top-level error ReferenceError: Cannot access 'WToolbarTextSelect' before initialization) fix: fixed critical circular dependencies Jan 17, 2025
@d3m1d0v d3m1d0v requested a review from makhnatkin January 31, 2025 14:01
makhnatkin
makhnatkin approved these changes Jan 31, 2025
View reviewed changes
src/toolbar/ToolbarGroup.tsx Outdated
@@ -5,14 +5,12 @@ import {cn} from '../classname';
import {ToolbarButton} from './ToolbarButton';
import {ToolbarButtonPopup} from './ToolbarButtonPopup';
import {ToolbarListButton} from './ToolbarListButton';
import {ToolbarBaseProps, ToolbarDataType, ToolbarGroupItemData} from './types';
import {ToolbarBaseProps, ToolbarDataType, ToolbarGroupData} from './types';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type?

src/toolbar/Toolbar.tsx Outdated
import {ToolbarButtonGroup, ToolbarGroupData} from './ToolbarGroup';
import {ToolbarBaseProps} from './types';
import {ToolbarButtonGroup} from './ToolbarGroup';
import {ToolbarProps} from './types';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d3m1d0v d3m1d0v d3m1d0v approved these changes

@makhnatkin makhnatkin makhnatkin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@obenjiro @d3m1d0v @gravity-ui-bot @makhnatkin