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

Add exportJSON to ArtificialNode #6107

Closed
wants to merge 1 commit into from
Closed

Add exportJSON to ArtificialNode #6107

wants to merge 1 commit into from

Conversation

thegreatercurve
Copy link
Contributor

@thegreatercurve thegreatercurve commented May 15, 2024 •
edited

Fix browser warnings for missing method on ArtificialNode.

@vercel Vercel
Copy link

vercel bot commented May 15, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2024 3:05pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2024 3:05pm

@facebook-github-bot facebook-github-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 May 15, 2024
@thegreatercurve thegreatercurve marked this pull request as ready for review May 15, 2024 15:05
@github-actions GitHub Actions
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 23.93 KB (0%) 479 ms (0%) 477 ms (+21.34% 🔺) 956 ms
packages/lexical-rich-text/dist/LexicalRichText.js 34.6 KB (+0.13% 🔺) 693 ms (+0.13% 🔺) 1.5 s (+13.08% 🔺) 2.1 s
packages/lexical-plain-text/dist/LexicalPlainText.js 34.57 KB (+0.18% 🔺) 692 ms (+0.18% 🔺) 1.3 s (-3.94% 🔽) 2 s

potatowagon
potatowagon approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@potatowagon potatowagon left a comment

Choose a reason for hiding this comment

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

thanks lgtm

@thegreatercurve thegreatercurve added this pull request to the merge queue May 15, 2024
@thegreatercurve thegreatercurve removed this pull request from the merge queue due to a manual request May 15, 2024
@thegreatercurve thegreatercurve added this pull request to the merge queue May 15, 2024
@potatowagon potatowagon mentioned this pull request May 15, 2024
Open
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 15, 2024
zurfyx
zurfyx requested changes May 15, 2024
View reviewed changes
packages/lexical/src/nodes/ArtificialNode.ts
@@ -20,4 +20,8 @@ export class ArtificialNode__DO_NOT_USE extends ElementNode {
const dom = document.createElement('div');
return dom;
}

exportJSON() {
return super.exportJSON();
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it throw an invariant instead please. This Node should not exist and should never land on any database.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a commit in #6048 that just ignores the warnings for these nodes, which is another approach that would preserve expected runtime errors without sacrificing DX 8cd0664

Copy link
Contributor

Choose a reason for hiding this comment

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

im not sure if changing to invariant would continue to raise errors. the browser warnings was reported to break the serialisation of the editor state on KP's usecase (T189214869), hence the goal of the PR to silence the warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a commit in #6048 that just ignores the warnings for these nodes, which is another approach that would preserve expected runtime errors without sacrificing DX 8cd0664

thanks for this, lgtm!

potatowagon
potatowagon reviewed May 15, 2024
View reviewed changes
packages/lexical/src/nodes/ArtificialNode.ts

exportJSON() {
return super.exportJSON();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

#5966 reports that there are a couple more unimplemented methods that are triggering console warnings. if u have bandwidth could u help add those in? if not ill be happy to add those in after this PR is merged

@thegreatercurve
Copy link
Contributor Author

Closing in favour of #6048

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

@etrepum etrepum etrepum left review comments

@potatowagon potatowagon potatowagon approved these changes

@zurfyx zurfyx zurfyx requested changes

@fantactuka fantactuka Awaiting requested review from fantactuka fantactuka is a code owner

@acywatson acywatson Awaiting requested review from acywatson acywatson is a code owner

@Fetz Fetz Awaiting requested review from Fetz Fetz is a code owner

@ivailop7 ivailop7 Awaiting requested review from ivailop7 ivailop7 is a code owner

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@thegreatercurve @etrepum @zurfyx @potatowagon @facebook-github-bot