Skip to content

[lexical-markdown] Refactor: Remove MarkdownShortcuts.ts Dependency on index.ts#7832

Merged
etrepum merged 4 commits intofacebook:mainfrom
jkjk822:patch-2
Sep 19, 2025
Merged

[lexical-markdown] Refactor: Remove MarkdownShortcuts.ts Dependency on index.ts#7832
etrepum merged 4 commits intofacebook:mainfrom
jkjk822:patch-2

Conversation

@jkjk822
Copy link
Copy Markdown
Contributor

@jkjk822 jkjk822 commented Sep 19, 2025

This import is unused and creates a cyclic dependency.

Description

Revival of #6474, which I forgot about. The cycle has been fixed through other changes, the only remaining piece is this dangling import.

Test plan

N/A

This import is unused and creates a cyclic dependency.
@vercel
Copy link
Copy Markdown

vercel bot commented Sep 19, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
lexical Ready Ready Preview Comment Sep 19, 2025 9:02pm
lexical-playground Ready Ready Preview Comment Sep 19, 2025 9:02pm

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2025
@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 19, 2025

It's not unused Error: packages/lexical-markdown/src/MarkdownShortcuts.ts(397,38): error TS2552: Cannot find name 'TRANSFORMERS'. Did you mean 'transformers'?

Practically speaking I don't think the circular reference is really an issue since it should get compiled out in the bundling process, but to break the cycle you'd probably want to move the definition of TRANSFORMERS (and the other arrays used to build it) into MarkdownTransformers.ts or a new file.

@jkjk822
Copy link
Copy Markdown
Contributor Author

jkjk822 commented Sep 19, 2025

Oops, I was looking at the wrong branch, I'll likely do a fix like my original commit in #6474 then.

Move transformer groups created in index.ts to MarkdownTransformers.ts, co-locating them with the member transformers and avoiding a cyclical dependency involving index.ts.
Copy link
Copy Markdown
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

It's failing eslint, the imports should be sorted. npm run lint -- --fix then npm run prettier:fix should fix that (normally this sort of thing is addressed on commit by the git hooks that are installed by husky)

@jkjk822
Copy link
Copy Markdown
Contributor Author

jkjk822 commented Sep 19, 2025

Yea, I've been fighting a lint error this whole time. Any ideas? The file is actually present but it fails with:
Error: Failed to load plugin 'lexical' declared in '.eslintrc.js': Cannot find module '../../../scripts/shared/PackageMetadata.js'

I might just do the linting manually and reclone the repo for my next change if I can't fix it.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 19, 2025

I've never seen it error like that. Are you using npm to install everything? Are you invoking eslint from the repo's node_modules and not some global install?

@jkjk822
Copy link
Copy Markdown
Contributor Author

jkjk822 commented Sep 19, 2025

Huh, bizarrely the issue seems to be that it is running from the node_modules directory.

I get

Error: Failed to load plugin 'lexical' declared in '.eslintrc.js': Cannot find module '../../../scripts/shared/PackageMetadata.js'
Require stack:
- /usr/home/jkjk822/github/lexical/node_modules/eslint-plugin-lexical/src/rules/no-imports-from-self.js
- /usr/home/jkjk822/github/lexical/node_modules/eslint-plugin-lexical/src/rules/index.js
- /usr/home/jkjk822/github/lexical/node_modules/eslint-plugin-lexical/src/index.js
- /usr/home/jkjk822/github/lexical/node_modules/@eslint/eslintrc/dist/eslintrc.cjs
Referenced from: /usr/home/jkjk822/github/lexical/.eslintrc.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1212:15)
    at Module._load (node:internal/modules/cjs/loader:1043:27)
    at Module.require (node:internal/modules/cjs/loader:1298:19)
    at require (node:internal/modules/helpers:182:18)
    at Object.<anonymous> (/usr/home/jkjk822/github/lexical/node_modules/eslint-plugin-lexical/src/rules/no-imports-from-self.js:16:27)
    at Module._compile (node:internal/modules/cjs/loader:1529:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)
    at Module.load (node:internal/modules/cjs/loader:1275:32)
    at Module._load (node:internal/modules/cjs/loader:1096:12)
    at Module.require (node:internal/modules/cjs/loader:1298:19)

So ../../../scripts/shared/PackageMetadata.js is correct for the original location of eslint-plugin/src/rules/no-imports-from-self.js, but not for the node_modules location of node_modules/eslint-plugin-lexical/src/rules/no-imports-from-self.js as it is one directory deeper.

If I manually add an extra .. to the file under node_modules, npm run lint works as expected.


I have a workaround so not a big issue anymore, but any idea why this is the case? I started fresh with git clone, npm install and still had this same issue.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 19, 2025

That's interesting, what platform are you using? Maybe it has something to do with whether your platform supports symlinks or not. I've never noticed an issue on macOS or Linux.

@jkjk822
Copy link
Copy Markdown
Contributor Author

jkjk822 commented Sep 19, 2025

Just Linux, though a custom distro. I think I found the issue though, and why it worked for me previously - seems like npm v9 changed default symlink behavior: https://stackoverflow.com/q/74460729

Indeed running npm install --install-links=false allows npm run lint to work in a fresh repo.

Might be worth doing a quick check to see if this is something Lexical needs to override generally or just something weird on my system. If you remove your package-lock.json and node_modules/ dir, see if you can repro by running npm install or npm install --install-links

Thanks for approval!

etrepum added a commit to etrepum/lexical that referenced this pull request Sep 19, 2025
@etrepum etrepum added this pull request to the merge queue Sep 19, 2025
Merged via the queue into facebook:main with commit 8a89107 Sep 19, 2025
39 checks passed
@jkjk822 jkjk822 deleted the patch-2 branch September 19, 2025 22:31
This was referenced Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants