feat: support self-identifying workspace schemas#39
Conversation
5de1b92 to
5e66f7a
Compare
|
I forgot that the |
5e66f7a to
0254094
Compare
|
Hi @jdesrosiers , Since PR #32 has been merged, I have successfully rebased the branch on top of main, resolved the conflicts, and ensured all tests are green. It is ready for review. |
jdesrosiers
left a comment
There was a problem hiding this comment.
Honestly, this needs a lot of work. I left a few comments, but it's just a start and is just code quality issues. I'll take a closer look at the functionality after you clean things up a little.
This functionality shouldn't be in its own class. It needs to be part of the SchemaStore functionality. So, for the next iteration, try to clean things up and move the functionality to SchemaStore.
| // Clear socket queue from duplicate open/watch notifications | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
There was a problem hiding this comment.
This isn't acceptable. Find a way to do this without an arbitrary wait.
There was a problem hiding this comment.
I will remove the setTimeout blocks from the tests.
To test the startup discovery without a sleep, would it make sense to have the test client listen for the server's "window/logMessage" (e.g. waiting for "Scanning completed") before triggering the test validation? Or is there a more standard LSP event or testing helper in this codebase that I should use to detect when the server has settled?
| afterEach(async () => { | ||
| await client.stop(); | ||
| try { | ||
| unregisterSchema(schemaId); |
There was a problem hiding this comment.
The server should be managing schema registration, not the test.
There was a problem hiding this comment.
I will remove the manual unregisterSchema call from the tests.
To ensure the server cleans up registered workspace schemas when it shuts down, would it make sense to add a server.onShutdown listener inside SchemaStore that unregisters them? Or is there a different server lifecycle hook you would recommend for this?
| // 2. Create and open an instance file that references the local schema by its $id | ||
| const diagnosticsPromise = new Promise<Diagnostic[]>((resolve) => { | ||
| client.onNotification("textDocument/publishDiagnostics", (params: PublishDiagnosticsParams) => { | ||
| if (params.uri.endsWith("instance.json")) { |
There was a problem hiding this comment.
writeDocument returns the file URI. Use that to match on params.uri.
| const diagnostics = await diagnosticsPromise; | ||
| expect(diagnostics).toHaveLength(1); | ||
| expect(diagnostics[0].message).toContain("Expected a string"); |
There was a problem hiding this comment.
Have a look at similar assertion in SchemaValidation.test.ts and try to match that code style.
| // 3. Update the schema to allow a number for "foo" | ||
| const updatedDiagnosticsPromise = new Promise<Diagnostic[]>((resolve) => { | ||
| client.onNotification("textDocument/publishDiagnostics", (params: PublishDiagnosticsParams) => { | ||
| if (params.uri.endsWith("instance.json")) { | ||
| resolve(params.diagnostics); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| await client.writeDocument("my-schema.json", `{ | ||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | ||
| "$id": "${schemaId}", | ||
| "type": "object", | ||
| "properties": { | ||
| "foo": { "type": "number" } | ||
| } | ||
| }`); | ||
|
|
||
| // Wait a brief moment for the watched file event and write to settle on Windows | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| const updatedDiagnostics = await updatedDiagnosticsPromise; | ||
| expect(updatedDiagnostics).toHaveLength(0); |
There was a problem hiding this comment.
This test is doing too much. This part should be a separate test.
| server.onInitialize((params) => { | ||
| if (params.workspaceFolders) { | ||
| this.workspaceFolders = params.workspaceFolders.map((folder) => folder.uri); | ||
| } else if (params.rootUri) { | ||
| this.workspaceFolders = [params.rootUri]; | ||
| } | ||
| return { capabilities: {} }; | ||
| }); |
There was a problem hiding this comment.
Workspace takes care of this. You don't need to do it again. I wanted to wait until the other PR was merged specifically so you don't need to do this.
There was a problem hiding this comment.
Understood. I will remove the duplicate onInitialize listener and use this.workspace.workspaceFolders directly.
| if (!fileUri.endsWith(".json") && !fileUri.endsWith(".jsonc")) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is unnecessary. The watch was configured to only send .json and .jsonc changes.
| // change.type is: 1 = Created, 2 = Changed, 3 = Deleted | ||
| const changeType = change.type as FileChangeType; | ||
|
|
||
| if (changeType === 3) { |
There was a problem hiding this comment.
Sloppy
| // change.type is: 1 = Created, 2 = Changed, 3 = Deleted | |
| const changeType = change.type as FileChangeType; | |
| if (changeType === 3) { | |
| if (change.type === FileChangeType.Deleted) { |
| try { | ||
| const text = await fs.readFile(filePath, "utf-8"); | ||
| const ast = jsonc.parseTree(text); | ||
| if (!ast || ast.type !== "object") { |
There was a problem hiding this comment.
Use optional chaining.
There was a problem hiding this comment.
I will use optional chaining (ast?.type and schemaNode?.type) for the check.
| try { | ||
| unregisterSchema(id); | ||
| } catch { | ||
| // Ignore if not registered | ||
| } |
There was a problem hiding this comment.
unregisterSchema doesn't throw.
There was a problem hiding this comment.
I will remove the redundant try-catch block and call unregisterSchema(id) directly.
|
Hi @jdesrosiers, Thank you for the feedback! I will consolidate all of the workspace scanning, file watching, and schema registration logic directly into |
80c0ca9 to
730ea89
Compare
|
Hi @jdesrosiers, I have refactored the PR according to your feedback:
Let me know what you think! |
- Implement startup scanning, registration under , and watcher lifecycle management. - Update and cache clearing and dependency mapping with URI normalization. - Correct and stabilize test assertions.
fce8d6a to
d702e53
Compare
d702e53 to
9287ee0
Compare
This PR introduces automatic discovery and lifecycle management of local workspace schemas. It detects JSON files with $schema and $id defined at the root and keeps the schema registry in sync as files are added, updated, or removed.
Key Changes
SelfIdentifyingSchemas: Added automatic workspace scanning on startup, dynamic schema registration by$idusing@hyperjump/json-schema, and file watching to keep the registry in sync. Also added three new integration tests.SchemaStore.ts: Added tracking for workspace schema$ids. IntroducedunregisterSchemainclear()to prevent@hyperjump/json-schemafrom serving stale schemas from its global cache after files are modified or removed.JsonDocument.ts: UpdateddependsOn()to recognize dependencies between JSON documents and workspace schema$ids. Also added URI normalization (normalizeIri) to avoid OS-specific path mismatches.build-server.ts: Initialized the workspace schema manager beforeDiagnosticsto ensure schemas are registered before the initial validation pass.TestClient.ts: Fixed path resolution indeleteDocument(resolving aTypeError) and added watcher notifications for deleted files.SchemaValidation.test.ts: Improved test reliability by adding short delays to eliminate race conditions caused by duplicate diagnostic notification queues.