Conversation
|
@srivastava-diya, please do the first review |
| import { getSchema } from "@hyperjump/json-schema/experimental"; | ||
| import * as Schema from "@hyperjump/browser"; | ||
| import * as Instance from "@hyperjump/json-schema/instance/experimental"; | ||
| import jsonStringify from "json-stringify-deterministic"; |
There was a problem hiding this comment.
hey @Ahmedmhmud please remove the unused variables, as oxlint gives warning.
| getSiblingKeywordValue: ( | ||
| schemaLocation: string, | ||
| siblingKeywordUri: string | ||
| ) => { keywordLocation: string; keywordValue: unknown } | undefined; |
There was a problem hiding this comment.
please run npm run lint command to know if there are any linting issues and fix them because upon raising a PR it causes build failure, you can also use npx oxlint src --fix to fix linting issues automatically if they are fixable.
| /** @type ErrorHandler */ | ||
| const minimumErrorHandler = async (normalizedErrors, instance, localization) => { | ||
| const minimumErrorHandler = (normalizedErrors, instance, localization, resolver) => { | ||
| /** @type {{ |
There was a problem hiding this comment.
here the JSDoc on if actually does nothing so i think we can either remove it or it can be moved to annotate resolver as a variable.
| /** @type ErrorHandler */ | ||
| const containsErrorHandler = async (normalizedErrors, instance, localization) => { | ||
| const containsErrorHandler = (normalizedErrors, instance, localization, resolver) => { | ||
| /** @type {{ |
There was a problem hiding this comment.
JSDoc @type above the if does nothing, it can be removed.
|
Hi @jdesrosiers, @srivastava-diya |
| * readable messages. | ||
| */ | ||
| export type ErrorHandler = (normalizedErrors: InstanceOutput, instance: JsonNode, localization: Localization) => Promise<ErrorObject[]>; | ||
| export type ErrorHandler = (normalizedErrors: InstanceOutput, instance: JsonNode, localization: Localization, resolver?: ErrorResolver) => ErrorObject[]; |
There was a problem hiding this comment.
resolver is now always required by handlers that use it, but it's still typed as optional. I think we make this required which removes the need for defensive checks entirely from the handlers.
| * build errors in applicator error handlers. | ||
| */ | ||
| export const getErrors: (normalizedErrors: NormalizedOutput, instance: JsonNode, localization: Localization) => Promise<ErrorObject[]>; | ||
| export const getErrors: (normalizedErrors: NormalizedOutput, instance: JsonNode, localization: Localization, resolver?: ErrorResolver) => ErrorObject[]; |
There was a problem hiding this comment.
same goes for this one, we can make the resolver required here.
Ahmedmhmud
force-pushed
the
making-getErrors-sync
branch
from
0ece108 to
d24ba98
Compare
|
I made resolver required in ErrorHandler and getErrors, then removed resolver checks from all handlers that use it. |
|
hey @jdesrosiers all my comments have been addressed, LGTM from my side. |
3 participants
This PR removes async schema value lookups from the error formatting path and makes getErrors synchronous by resolving keyword values from the compiled AST to using it so it can be used in
@hyperjump/json-schemaas we need here (#204).I replaced async schema reads with two AST resolver functions: getCompiledKeywordValue() for normal keyword lookups by schemaLocation, and getSiblingKeywordValue() for sibling keywords in the same parent node (like draft-04 min/max exclusivity and contains min/max contains). Then getErrors passes this resolver to handlers, so the whole error handling path is synchronous and no longer depends on await getSchema().
getErrors now passes an AST resolver to handlers, including recursive ones, and sibling-dependent keywords use sibling lookup from the same parent AST node. I also adjusted handlers for compiled value shapes (like regex, draft-04 tuples, and const/enum JSON strings), then updated types. All tests are still passing leaves us with the same behavior.