kolibri icon indicating copy to clipboard operation
kolibri copied to clipboard

standardize validator functions to all `Object` props

Open indirectlylit opened this issue 3 years ago • 10 comments

Observed behavior

Following up from: https://github.com/learningequality/kolibri/issues/8640, https://github.com/learningequality/kolibri/pull/8878, and https://github.com/learningequality/kolibri/pull/8902 which added validation functions.

There are a large number of Object-type props with no validator function. (Search the codebase using a multi-line regex to help find them: {[^}]*type: Object[^}]*})

Normal component props are well-documented using type, required, and sometimes validator. The keys of Object props are as much a part of the component API as the other ones, but they are often under-documented.

Even when validation of Object props is done in Kolibri, it is performed in an ad-hoc manner with little consistency and the logging is less useful because usually Vue will simply say the validator failed without specifying why. Some examples:

https://github.com/learningequality/kolibri/blob/c3238a85915a75386150c5f1191de6735e495d1c/kolibri/core/assets/src/views/sync/FacilityAdminCredentialsForm.vue#L54-L60

https://github.com/learningequality/kolibri/blob/c3238a85915a75386150c5f1191de6735e495d1c/kolibri/plugins/setup_wizard/assets/src/views/importFacility/SelectFacilityForm.vue#L67-L73

https://github.com/learningequality/kolibri/blob/c3238a85915a75386150c5f1191de6735e495d1c/kolibri/plugins/learn/assets/src/views/EmbeddedSidePanel/SelectGroup.vue#L72-L79

Expected behavior

Component object props should have a validator and fill in default values which should simplify internal logic of components using these props

indirectlylit avatar Dec 13 '21 06:12 indirectlylit

Is this issue still open? I would like to work on this

nikkuAg avatar Feb 09 '24 16:02 nikkuAg

Hi, @nikkuAg, yes, thanks for volunteering

MisRob avatar Feb 12 '24 07:02 MisRob

This seems to be relevant https://github.com/learningequality/kolibri/pull/8902/files#diff-294e45dbea9c24d93f492cafd5cf86476c419f189cbb3bf0d77f7ffb9f8ac220

MisRob avatar Feb 12 '24 07:02 MisRob

This seems to be relevant

yes, that was the intent - thank you

Introduces two new functions, validateObject and objectWithDefaults, which bring Vue prop-like validation and default behavior to nested javascript objects. This can be used with component props, vuex and API validation, and other scenarios

a stopgap for lack of TypeScript, perhaps :)

indirectlylit avatar Feb 12 '24 23:02 indirectlylit

a stopgap for lack of TypeScript

Except we could use this for runtime validation, which TypeScript does not provide!

rtibbles avatar Feb 13 '24 02:02 rtibbles

Hi, I am not entirely able to understand what I have to do.

nikkuAg avatar Mar 01 '24 18:03 nikkuAg

The basic idea is to use the Object validation specification work that was done in the referenced PRs, and use it to fully document each of the cases where an Object type is used as a prop.

This allows for better specification of the intended properties and shape of the prop, and to give more semantic error messages when these expectations are violated.

I would start by looking at one of the examples listed in the issue, and replacing the validator function with the Object validation specification.

rtibbles avatar Mar 04 '24 17:03 rtibbles

Hi @nikkuAg, are you still planning to work on this at some point or would it be better to unassign?

MisRob avatar Apr 05 '24 13:04 MisRob

Hi @MisRob, I'm currently occupied with another task, but once I complete it, I'll be able to pick this up. Therefore, I believe it would be best to unassign me for now so that someone else can take over.

nikkuAg avatar Apr 07 '24 08:04 nikkuAg

Okay @nikkuAg, thank you, let us know any time if you'd like to return to this

MisRob avatar Apr 08 '24 03:04 MisRob