Repository navigation
Don't use AJV. Use JSON. #96
Description
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.
Activity
lance commentedon 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 commentedon 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 commentedon 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 commentedon 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
isValidAgainstSchemamethod. Perhaps in avalidate_ce.jsfile rather than the main spec file. Is that fine?lance commentedon 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 commentedon May 1, 2020
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 commentedon 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
CloudEventclass 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 commentedon 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.