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

Don't use AJV. Use JSON. #96

Closed
grant opened this issue Apr 30, 2020 · 8 comments
Closed

Don't use AJV. Use JSON. #96

grant opened this issue Apr 30, 2020 · 8 comments

Comments

@grant
Copy link
Member

grant commented Apr 30, 2020

This SDK uses a library called AJV for schema validation. However, it makes creating and using CloudEvent objects very complicated through a set of setters and getters.

It would be a lot simpler to be able to directly create CloudEvent objects given a JSON object. Further field validation could be done externally through a different function rather than a ajv.compile(schema) and external spec.

@lance
Copy link
Member

lance commented May 1, 2020

Can you help me understand why you think that JSON schema validation makes creating CloudEvent instances difficult? I don't follow.

@grant
Copy link
Member Author

grant commented May 1, 2020

The issue is around AJV.

https://github.com/cloudevents/sdk-javascript/blob/master/lib/specs/spec_1.js#L36

which expects a file like:

https://github.com/cloudevents/sdk-javascript/blob/master/ext/spec_1.json

and uses boilerplate like this:

https://github.com/cloudevents/sdk-javascript/blob/master/lib/specs/spec_1.js#L111-L201


Rather than creating this boilerplate, I suggest we remove AJV. This will make it possible to do things like fix this issue around directly using JSON: #65

@lance
Copy link
Member

lance commented May 1, 2020

However, it looks to me like that's not what AJV is used for in this case. If you look at the commit history of that file, you can see that it appears to have been hand written. In this case, AJV is not being used to generate code, rather it is being used to validate input, which I think is a reasonable usage of the module since that input may be incorrect.

It's certainly reasonable to want to simplify this file, but I don't think removing data validation checks is necessarily required to do so.

@grant
Copy link
Member Author

grant commented May 1, 2020

OK, so if we want to keep the feature to validate the input, I suggest we use a different tool than AJV for the isValidAgainstSchema method. Perhaps in a validate_ce.js file rather than the main spec file. Is that fine?

@lance
Copy link
Member

lance commented May 1, 2020

I don't have any particular love or hate for AJV. But do you have another suggested tool for schema validation? AJV gets 31 million downloads a week, so I think it's probably pretty well liked.

@grant
Copy link
Member Author

grant commented May 1, 2020

I don't have any particular love or hate for AJV. But do you have another suggested tool for schema validation? AJV get's 31 million downloads a week, so I think it's probably pretty well liked.

I'll look into it. It might just be the way we use the library with our current format and not the validator itself.

@lance
Copy link
Member

lance commented May 29, 2020

@grant I think that the schema validator is actually really important and would be a lot of work to replace manually (and would not be as fast/efficient as AJV). It is sometimes valuable to have the capability to determine if a CloudEvent is actually valid per the specification and that's what this does. I can see an argument for decoupling the validator from the CloudEvent class itself and exposing it as a function. But I don't see value in removing this capability altogether. I think this issue should be closed, or perhaps you have something in mind that I'm not aware of.

@grant
Copy link
Member Author

grant commented May 29, 2020

Yeah, the usage now is a lot cleaner than it was before.
It does take 60% of the bundle size, so I was a little concerned if we're trying to slim this package down.
Maybe it could an optional dependency in the future.

Should be fine to keep until there's a better suggestion.

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