Proposal for refactor

[maldimirov]

While trying to better understand JSON Schema I ended up with this Q/A thread https://github.com/json-schema-org/json-schema-spec/issues/733 which explained a lot in how JSON Schema validation should work. Taking that into consideration I think enjoi is due for a major refactor. At the moment enjoy validates against e.g. required and properties only if "type": "object". And all validation keywords should be independently taken into consideration and validated against.

I am willing to work on this if you would consider a later PR. Also since it will be a major refactor would you consider me changing the testing library with jest. I've been using it for some time now and it's proven very flexible and versatile.

[tlivings]

Happy for any contributions but not sure why that includes switching test framework.

Is that just because you are more comfortable with jest than tape?

[tlivings]

In addition, will this address some known issue? I read the issue you opened but it seems more based on principle than resolving any particular issue.

i.e properties doesn’t make any sense when it appears on a type other than object.

[maldimirov]

Mostly it's out of principle, yes. We had only 1 issue with enjoy where we had a mistyped schema. Roughly something like the following:

{
  oneOf: [...],
  type: 'object',
  properties: {...}
}

This schema didn't care about anything after oneOf which caused unexpected behavior, and was very hard to find.

As you pointed out properties doesn't make any sense outside the scope of an object type, but that's the way JSON Schema was defined. If you have a schema with only the properties keyword it will validate only object values against the keyword, but allow everything else that is not an object.

[tlivings]

Thanks for the explanation. It’s rather surprising that the meta schema does not catch this (in general).

[maldimirov]

Well I also only learned this by the github thread, not by the documentation itself. I agree documentation is vague on this topic and should be more explicit.

[tlivings]

So the question is do we want to enforce the correct behaviors and create more sane schemas or be as vague as the spec?

[maldimirov]

I guess. At this point the choice is entirely yours. Since these problems we have switched to use ajv, which follows JSON Schema very strictly and even has a schema validator. It probably isn't worth it, until someone else complains (afaik I was the only one until now). And such a refactor could break the familiar behavior for people already using it.

[maldimirov]

And just to answer the question about the test framework. It is because it's hard to use tape since you have to pre-register the number of tests. And it's hard to develop new functionality and test only that, since all tests are run every time. Or at least I couldn't find an option to run only 1 test. Both of these are not issues with jest and overall it's a pretty neat JS testing framework.

[tlivings]

That’s actually not true. Tape can run an arbitrary number of assertions and end a test with t.end().

However, planning your assertions also ensures the expectations of a test are met, and fail when they are not.

Tests can be skipped, certain suites run only, etc. You can also execute single tests files since it doesn’t require a test runner.

[tlivings]

Here is a nice little write up that I generally agree with too:

https://link.medium.com/O4HC9ZjWZW

[tlivings]

Accidentally closed, oops.

Anyway I am not saying that changing the test framework is out of the question - but I’ve come to appreciate the simplicity and flexibility of tape and generally take a “if it ain’t broke don’t fix it” mentality on such changes.

But if changing the testing framework is going to make it easier for contributors then maybe that’s a good thing too (I just personally think tape is easy enough to use).

[maldimirov]

I guess it is my fault too, for not trying to more familiar with tape.

[svrnwnsch]

What is the states of the refactor? If major refactor of the code is done it would be nice if at the same moment #79 gets closed and joi version 16.X is supported.

[maldimirov]

Currently the refactor is on pause. We have moved on to ajv and don't depend any more on enjoy as a library for the project. That is why I can work on this only in my spare time. Since @tlivings was not very explicit on whether such a PR would be accepted I've not spent any time on this for a while.

[tlivings]

79 is being planned soon. Also happy to see a PR.

[tlivings]

Also, I understand why you'd use ajv — it is, after all, designed for JSON schema validation. enjoi is designed to perform joi validation and use JSON schema (from swagger files for example), to define the joi schema.

[maldimirov]

If that is the purpose of enjoi, do you think it has any value for the project to follow strictly the JSON Schema validation rules?

[tlivings]

I do, where possible and supported by joi. I’ve appreciated all the work from contributions like yours to help make it follow json schema better!

[tlivings]

I've started to work on it but there are a lot of breaking changes. I've asked for help with it but in absence of contribution it is a bit slower going than I would like.

on the other hand i believe the best choice to move to Fastify (which has json schema validation from the box) and forget about that old and proprietary hapi, but i'd like to see some news here

Regarding that statement... This module is really designed for using JSON schema with hapi based servers. Hapi, to me, remains the best node.js framework for enterprise environments with large distributed teams. I would not characterize it as "old" ...

That being said, fastify is a great framework to use if you don't need the benefits hapi provides. And yes, if you aren't using hapi you are less likely to need to use this module.