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

Great library, could use some improvements to the developer experience #71

Open
antonio-ivanovski opened this issue Nov 29, 2024 · 1 comment

Comments

@antonio-ivanovski
Copy link

Hello, great library you have build supporting great amount of JSON Schema spec. I was considering switching from Ajv to this library in order to get away from their schema-no-backward compatibility (eg. 2020-12 cannot be used together with 2019-09 or draft-7). Testing some schemas on the playground, this library passed those mixed-schema-version checks and gave me confidence that this might be it.

But I have some gripes that make this library unusable for me

  • No way to check if schema is registered. There is just the method of registerSchema. I need to be able to check if schema is loaded before registering it.
  • No way to get schema that is behind authentication or served via non-standard media type. I can use to some extent addMediaTypePlugin, but working with dynamic set of of schemas, you are destined to stumble on schemas that don't have the required type for the library
  • No way to pass the JSON schema directly to the validate method, you are required to first registerSchema(jsonSchema, jsonSchema.$id). This coupled with the inability to check if a schema has been registered leads to bad developer experience.
  • Single validator instance per application. Having the ability to separate maybe 2 instance of validator makes sense. Considering that there could be clash of instances between packages. Also maybe I want to pass the validate instance outside of my package and others to use it.

Hope this feedback is helpful!

@jdesrosiers
Copy link
Collaborator

Thanks for your feedback. Different people can have different ideas about what's essential, so it's valuable to hear different perspectives. Some of these things I'm aware of and some I'm hearing for the first time.

I was considering switching from Ajv to this library in order to get away from their schema-no-backward compatibility (eg. 2020-12 cannot be used together with 2019-09 or draft-7).

I'm glad to hear this feature is useful to you. I don't think many implementations in any language get this right and I'm proud that mine does.

No way to check if schema is registered. There is just the method of registerSchema. I need to be able to check if schema is loaded before registering it.

This is not a need I've ever come across, nor has anyone else ever asked for it, but it's very easy to add. I'll do that soon. I'm curious what you're doing that you would need this for. I can provide a better API in the future if I understand the use-case.

No way to get schema that is behind authentication or served via non-standard media type. I can use to some extent addMediaTypePlugin, but working with dynamic set of of schemas, you are destined to stumble on schemas that don't have the required type for the library

Actually, there is a way to get a schema behind authentication. You can use a custom UriSchemePlugin to override how the request is made. Something like this,

import { addUriSchemePlugin, acceptableMediaTypes } from "@hyperjump/browser";

addUriSchemePlugin("https", {
  retrieve: async (uri) => {
    const response = await fetch(uri, {
      headers: {
        Authentication: "token goes here",
        Accept: acceptableMediaTypes()
      }
    });

    if (response.status !== 200) {
      throw Error(`${response.status}: Failed to retrieve ${uri}`);
    }

    return response;
  }
})

Ideally there would be an easier way to handle the special case of setting a header without having to provide a completely custom UriSchemePlugin, but I didn't think of an approach I liked, so this is the only way for now. This is definitely something I'd like to improve in v2.0.

As for non-standard media types, you can use custom MediaTypePlugins to allow whatever you like. But, it sounds like what you want is a wildcard MediaTypePlugin that would allow you to add a MediaTypePlugin for */* that serves as a default handler. That would allow you to support unknown media types. I'd advise against doing that. Assuming a response is JSON that says it's something else is considered unsafe and could be a security vulnerability. However, I'm willing to add support for wildcards because I think supporting something like application/*+json would be reasonable. That should be pretty easy to add. I'll do that soon.

No way to pass the JSON schema directly to the validate method, you are required to first registerSchema(jsonSchema, jsonSchema.$id). This coupled with the inability to check if a schema has been registered leads to bad developer experience.

I deliberately chose not to provide this function. I'm aware it's confusing for people coming from ajv, but I think it promotes good practice. Ephemeral or anonymous schemas don't come up much in the real world. When they're used that way, it almost always means the developer isn't managing their schemas effectively. Schemas used by an application are generally known ahead of time and should be registered when the application starts. Then when they're needed, you don't pull in the whole schema, you just use it's identifier (URI). When schemas are discovered dynamically, you don't register them at all, you retrieve them from their authority (usually a web server). Either way, you're always using a URI to refer to a schema.

I'll point out that the second argument of the registerSchema function is optional. It will automatically look for jsonSchema.$id. Passing it as the second argument is redundant. You only need it if the schema is anonymous (doesn't have $id) or you need a consistent URI to reference the schema by whether it has $id or not. For example, you can create the function you want.

export const validate = async (schema, instance, outputFormat) => {
  const uri = "https://example.com/ephemeral-schema";
  try {
    JsonSchema.registerSchema(schema, uri);
    return JsonSchema.validate(uri, instance, outputFormat);
  } finally {
    JsonSchema.unregisterSchema(uri);
  }
};

Single validator instance per application. Having the ability to separate maybe 2 instance of validator makes sense. Considering that there could be clash of instances between packages. Also maybe I want to pass the validate instance outside of my package and others to use it.

I agree 100%. There are things about this implementation that I think could be better, but this is the only thing I consider a big mistake. The main objective for v2.0 is to correct that design flaw.

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

No branches or pull requests

2 participants