Skip to content

Invalid missing schema#46

Open
jdesrosiers wants to merge 5 commits into
mainfrom
invalid-missing-schema
Open

Invalid missing schema#46
jdesrosiers wants to merge 5 commits into
mainfrom
invalid-missing-schema

Conversation

@jdesrosiers

Copy link
Copy Markdown
Collaborator

We left a TODO to handle the case where a JSON document uses an invalid schemas. I think this should cover that case.

I changed the TestClient to use @hyperjump/uri instead of vscode-uri because the incorrect normalization it does was causing problems with the tests on windows.

@jdesrosiers jdesrosiers force-pushed the invalid-missing-schema branch from 8b32c9b to 1f96100 Compare July 8, 2026 17:24
Comment on lines +136 to 137
} catch {
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i was wondering since this catch block is empty, a rejected compiledSchema always gets deleted regardless of whether cachedSchemaUri actually depends on schemaUri. For eg. we have 2 unrelated schemas, A (broken) and B (fine), editing B will evict A's cache entry too, resulting in recompile of A the next time it's used, so i just wanted to ask if this is intentional? like is it that a broken entry should get removed whenever anything changes so it can recompile?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I can't say I completely understand it, but I added a test that shows why it's necessary to clear the cache for invalid schemas. When trying things in the extension host, I'm not seeing any unnecessary recompiles, so I think it's fine. Let me know if you have a better solution.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh got it, i confused it with a different thing, my bad.

@srivastava-diya srivastava-diya force-pushed the invalid-missing-schema branch from 63b5208 to 6502aab Compare July 10, 2026 21:50
@srivastava-diya

srivastava-diya commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

I accidentally merged the style cleanup PR into main before this PR, so I updated the tests in this branch to match, which caused a conflict with main. I've resolved it by rebasing onto main.

If everything looks right to you shall i merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants