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

Syntactic synchronization, automatic parentheses, metadata in Ast #8893

Merged
merged 55 commits into from
Feb 2, 2024

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Jan 30, 2024

Pull Request Description

Important Notes

  • Metadata is now set and retrieved through accessors on the Ast objects.
  • Since some metadata edits need to take effect in real time (e.g. node dragging), new lower-overhead APIs (commitDirect, skipTreeRepair) are provided for careful use in certain cases.
  • The client is now bundled as ESM.
  • The build script cleans up git-untracked generated files in an outdated location, which fixes lint errors related to src/generated that may occur when switching branches.

Checklist

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

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 30, 2024
@kazcw kazcw self-assigned this Jan 30, 2024
@kazcw
Copy link
Contributor Author

kazcw commented Jan 30, 2024

(Stacked on #8891).

@kazcw kazcw linked an issue Jan 30, 2024 that may be closed by this pull request
app/gui2/shared/ast/parse.ts Outdated Show resolved Hide resolved
Comment on lines 41 to 44
export type NodeMetadataFields = {
position?: { x: number; y: number } | undefined
visualization?: VisualizationMetadata | undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't NodeMetadataFields also inherit MetadataFields? Nodes have externalId too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. Although all nodes are Asts, I wouldn't consider the externalId part of the node metadata. Their updates are currently handled separately, and in practice IDs have to be handled a bit specially--if an Ast has only one concrete child, that child should inherit the parent's externalId (this will be more important once we have AssignmentExpression nodes as a stable place to attach node metadata).

I'm not opposed to making MetadataFields the root of a metadata hierarchy if we find it would simplify anything, but currently (with the little metadata we have so far) I'm not seeing that to be the case.

app/gui2/shared/ast/tree.ts Outdated Show resolved Hide resolved
@kazcw kazcw mentioned this pull request Jan 30, 2024
5 tasks
This was linked to issues Jan 31, 2024
@kazcw kazcw mentioned this pull request Jan 31, 2024
5 tasks
Copy link
Collaborator

@somebody1234 somebody1234 left a comment

Choose a reason for hiding this comment

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

approving the ide-desktop changes, which changes the bundled format for the Electron entrypoint code from CJS to ESM. i haven't tested though, so make sure Electron still works fine for both ide and ide2.

@somebody1234
Copy link
Collaborator

is there a reason why we need the client to be ESM, btw?

@kazcw kazcw removed the CI: Ready to merge This PR is eligible for automatic merge label Feb 1, 2024
@kazcw
Copy link
Contributor Author

kazcw commented Feb 1, 2024

is there a reason why we need the client to be ESM, btw?

@somebody1234
The client now transitively depends on the WASM parser (because it's now used by the ydoc-server). With ESM bundling, we can use top-level await in the FFI module to allow other modules to use the WASM module safely--otherwise we would have to be careful to explicitly await the parser being loaded before doing anything that uses it.

@kazcw
Copy link
Contributor Author

kazcw commented Feb 1, 2024

Update (@farmaazon): The only blocker left is packaging. Here's what I've found:

  • We should bundle the WASM module as an asset and fetch it--that way it should work the same way in Vitest (node) / Electron (node, but with different restrictions on module loading) / browsers. The current runtime-checked approach doesn't work in Electron because Electron doesn't allow node modules to be imported dynamically. [Switching between different static WASM wrappers with export conditions seem like a viable alternative, but not any simpler I think.]

To get top level await working by switching to ESM bundling:

  • The main configuration changes are in this PR
  • We'll also need to upgrade to Electron 28 (to load ESM)
  • We might have to configure vite differently when building for Electron: Disable the topLevelAwait plugin and set build.target to something like esnext that lets native top-level await be used without polyfill.

Alternatively instead of top-level await we could explicitly await the parser loading, but we'd have to do so in various places--when starting the ydocserver, the app/mock, and tests that need the parser.

@farmaazon
Copy link
Contributor

The problem with switching to ESM is our ide-desktop/lib/client dependencies: many of them have require statements which esbuild does not handle well.

Trivial to replace:

  • opener
  • mime-types
  • portfinder

Harder to replace:

  • create-servers
  • tar

@kazcw kazcw added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels Feb 1, 2024
@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Feb 1, 2024
@mwu-tow mwu-tow merged commit 343a644 into develop Feb 2, 2024
22 of 25 checks passed
@mwu-tow mwu-tow deleted the wip/kw/metadata-in-tree branch February 2, 2024 09:22
@farmaazon farmaazon mentioned this pull request Feb 2, 2024
5 tasks
mwu-tow pushed a commit that referenced this pull request Feb 2, 2024
#8893 introduced a fatal regression for old IDE + new IDE also does not work on every system.

The fix:
1. Don't assume that WASM needed by ydocs server is bundled
2. Don't assume we know its exact name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. 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
4 participants