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

Metadata should not depend on absolute text spans #11390

Merged

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Oct 23, 2024

Pull Request Description

close #11304

Changelog:

  • update: add ide.snapshot optional metadata field containing the source code of the file
  • update: syncFileContents method tries to repair the metadata spans when it detects that the source file was edited and the received code does not match the code stored in the ide.snapshot metadata field

Important Notes

Tested in gui

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 23, 2024
@4e6 4e6 self-assigned this Oct 23, 2024
@@ -25,6 +25,7 @@
"debug": "^4.3.6",
"fast-diff": "^1.3.0",
"isomorphic-ws": "^5.0.0",
"js-base64": "^3.7.7",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pnpm add js-base64 caused some disturbance in pnpm-lock.yaml

ide: { ...this.syncedMeta.ide, node: newMetadata },
ide: {
...this.syncedMeta.ide,
...newSnapshot,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saving an updated code snapshot here

contentsReceived,
this.syncedCode ?? undefined,
unsyncedIdMap,
this.syncedMeta?.ide?.node,
Copy link
Contributor Author

@4e6 4e6 Oct 24, 2024

Choose a reason for hiding this comment

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

Providing the newCode argument to save the updated code snapshot after the successful metadata recovery

@4e6 4e6 marked this pull request as ready for review October 24, 2024 16:53
@@ -483,6 +484,14 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
}
}

private static encodeCodeSnapshot(code: string): string {
return Base64.encode(code)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could compress it with some fast (not necessarily super-efficient) widely-available algo, like gzip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a nodejs zlib module. But it will require adding zlib support for the GraalVM version. I'll implement it as a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed as #11420

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Oct 28, 2024
@mergify mergify bot merged commit db0582d into develop Oct 28, 2024
36 of 37 checks passed
@mergify mergify bot deleted the wip/db/11304-metadata-should-not-depend-on-absolute-text-spans branch October 28, 2024 09:41
@JaroslavTulach
Copy link
Member

The description of #11304 issue states following constraints. Were they all implemented by #11390? I am not sure:

as it is hard to estimate what impact of changes to UUID system would have on the language server protocol - keep UUID as they are now

OK, I see no changes to the UUID system in this PR.

as it is easy to see what impact change of a format of META-DATA section has - redesign the META-DATA section format

OK, I see addition of the snapshot field.

as it is possible to design a META-DATA format that is resilient to user changes - just redesign the META-DATA section format so that it does not depend on absolute text spans.

OK, only the snapshot field is added as far as I can see.

don't use any 3rd party (Y.js) persistence format

OK, no Y.js dependency as far as I can see.

whatever happens don't execute the old version of the code, only the lastest, newest one

Is this tested somewhere?

metadata section should be a comment

I don't see any change making the META-DATA section a comment.

I am not sure about the last two. @4e6 can you confirm these constrains were both met?

@JaroslavTulach
Copy link
Member

Another goal of #11304 was:

the new META-DATA section format will be versioned - once it is found insufficient ... we design new format

I don't see any signs of versioning, @4e6. Given you already plan to change the format in

there is a need to differentiate between the snapshot uncompressed and compressed by zlib. What are your plans to do this?

@4e6 4e6 mentioned this pull request Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata should not depend on (absolute) text spans
4 participants