Skip to content

Commit

Permalink
heavily improve performance when there are many events
Browse files Browse the repository at this point in the history
  • Loading branch information
@dmonad
dmonad committed Jun 15, 2023
1 parent aedd4c8 commit 885a740
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions src/utils/YEvent.js
View file
Expand Up @@ -44,6 +44,10 @@ export class YEvent {
* @type {null | Array<{ insert?: string | Array<any> | object | AbstractType<any>, retain?: number, delete?: number, attributes?: Object<string, any> }>}
*/
this._delta = null
/**
* @type {Array<string|number>|null}
*/
this._path = null
}

/**
Expand All @@ -60,8 +64,7 @@ export class YEvent {
* type === event.target // => true
*/
get path () {
// @ts-ignore _item is defined because target is integrated
return getPathTo(this.currentTarget, this.target)
return this._path || (this._path = getPathTo(this.currentTarget, this.target))
}

/**
Expand Down

11 comments on commit 885a740

@ulion
Copy link

@ulion ulion commented on 885a740 Jun 16, 2023

Choose a reason for hiding this comment

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

could this cause sync issue? my recent app build are encountered serious sync issue.

@dmonad
Copy link
Member Author

@dmonad dmonad commented on 885a740 Jun 16, 2023

Choose a reason for hiding this comment

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

Unlikely. Is your error message related to this code?

@ulion
Copy link

@ulion ulion commented on 885a740 Jun 16, 2023 •
edited

Choose a reason for hiding this comment

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

I am using some old slate-yjs modified version, the code is stable in prod env for years, since the build use yjs 13.6.4, the sync result many failure, wrong path, all kinds of, can not find path of slate node. rollback to 13.6.2 back to normal. all sentry errors came from yjs 13.6.4 built app. 13.6.3 and 13.6.4 is suspicious anyway.

@jvandenaardweg
Copy link

Choose a reason for hiding this comment

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

Also experiencing the same. Just upgrade from 13.6.2 to the 13.6.4 and experience weird sync issues indeed. NodeJS just hanging/locking. But no errors.

Also confirmed the behaviour is present in 13.6.3. So I don't think the change in this commit is the issue.

Switching back to 13.6.2 also fixes the issues.

@dmonad
Copy link
Member Author

@dmonad dmonad commented on 885a740 Jun 16, 2023

Choose a reason for hiding this comment

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

Can you please open a ticket with the specific error message you are seeing?

@dmonad
Copy link
Member Author

@dmonad dmonad commented on 885a740 Jun 16, 2023

Choose a reason for hiding this comment

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

FYI @NilSet

@pietrodn
Copy link

Choose a reason for hiding this comment

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

I found a bug related to this change in 13.6.4. Probably unrelated since 13.6.3 is NOT broken, as @jvandenaardweg says.
#544

@jvandenaardweg
Copy link

@jvandenaardweg jvandenaardweg commented on 885a740 Jun 23, 2023 •
edited

Choose a reason for hiding this comment

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

@ulion can you create an issue with the Sentry errors you have? I'm also experiencing weird issues with 13.6.4+, but don't have error messages. My browser just hangs/locks on large (as in large history) documents of like 1MB+, without any error. So can't be of much help here, but would like to see this issue fixed :) Your error messages could help others to fix the issue.

Also just tried 13.6.5 (latest version as of today), but the issue is also there.

@ulion
Copy link

@ulion ulion commented on 885a740 Jun 23, 2023

Choose a reason for hiding this comment

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

@jvandenaardweg I believe my issue was fixed in 13.6.5, and indeed introduced by 13.6.4 the cache of the path, it is supposed already be fixed and 13.6.5 already released. do you mean you still have problem with 13.6.5?

@dmonad
Copy link
Member Author

@dmonad dmonad commented on 885a740 Jun 23, 2023

Choose a reason for hiding this comment

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

The latest release should fix this issue. If not, please create a new issue - a sample document, or at least an error trace would be helpful.

@jvandenaardweg
Copy link

@jvandenaardweg jvandenaardweg commented on 885a740 Jun 23, 2023 •
edited

Choose a reason for hiding this comment

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

do you mean you still have problem with 13.6.5

Yeah. But just to clarify, it's probably not related to the issue you had. My browser just hangs on large documents without any error, which is not happening on 13.6.2. I guess i'll have to digg deeper to get some sense on why this happens. I thought our issues might be related.

Will look into this later!

Please sign in to comment.