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.

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

refactor: add async query for dom mutation scenario #1621

Merged
merged 8 commits into from
Mar 10, 2023
Merged

refactor: add async query for dom mutation scenario #1621

merged 8 commits into from
Mar 10, 2023

Conversation

Saul-Mirone
Copy link
Collaborator

This PR adds async version of query functions to avoid the ambiguity requestAnimationFrame call when updating dom.

@vercel
Copy link

vercel bot commented Mar 9, 2023

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

Name Status Preview Comments Updated
blocksuite ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 2:06AM (UTC)
blocksuite-react ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 2:06AM (UTC)

@codesandbox
Copy link

codesandbox bot commented Mar 9, 2023

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Copy link
Member

@lawvs lawvs left a comment

Choose a reason for hiding this comment

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

I try to deprecate the concept of richText. Therefore, I think asyncSetVRangeForVirgo or asyncSetVRange would be better.

See #1626

import type { ExtendedModel } from './types.js';

export function asyncSetVRange(model: BaseBlockModel, vRange: VRange) {
asyncGetRichTextByModel(model)
.then(richText => {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can async/await syntax be used here, I think the code format will be more uniform

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't use the asyn function here is because I want to handle the promise error in the function and make sure when it is called, users won't need to handle it manually.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think it is equivalent to write like this:

async function asyncSetVRange(model: BaseBlockModel, vRange: VRange) {
  try {
    const richText = await asyncGetRichTextByModel(model)
    richText?.vEditor?.setVRange(vRange);
  } catch (e) {
    throw e;
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

This try/catch looks redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is equivalent to write like this:

async function asyncSetVRange(model: BaseBlockModel, vRange: VRange) {

  try {

    const richText = await asyncGetRichTextByModel(model)

    richText?.vEditor?.setVRange(vRange);

  } catch (e) {

    throw e;

  }

}

IMO they won't be equal. When you're using try catch in an async function, it will reject the error, and if the place where it called doesn't catch it, it will be a floating promise error. But, nevermind, it's not that important in this case.

Copy link
Member

@doodlewind doodlewind left a comment

Choose a reason for hiding this comment

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

Great work!

@doodlewind doodlewind merged commit 29f3a9a into toeverything:master Mar 10, 2023
@Saul-Mirone Saul-Mirone deleted the add-async-query branch March 10, 2023 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:refactor Refactoring on codebase
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants