feat(next): string validation #118
Conversation
lukad
force-pushed
the
devxp-2538-validate-strings
branch
from
fa53f81 to
f473cbf
Compare
this the schema will not have to be cast to NonBooleanJsfSchema
lukad
force-pushed
the
devxp-2538-validate-strings
branch
from
768dbab to
37385ec
Compare
thien-remote
requested review from
dragidavid and
thien-remote
and removed request for
dragidavid
There was a problem hiding this comment.
Left a few comments but largely I think this is a good approach. I guess we'll see how everything falls into place as we work on the upcoming parts.
Last question about testing:
I assume when it comes to testing, we should run the tests from v0 against the new code + run the new ones we write, right?
|
Approving this as it seems we all agree that this is a good starting point! Thanks for adding it Luka 🙌 |
|
@dragidavid I just noticed I missed your question regarding tests.
The new tests should all pass. While we continue building |
| * Each property is validated with `validateSchema`. | ||
| */ | ||
| export function validateObject(value: SchemaValue, schema: NonBooleanJsfSchema): ValidationError[] { | ||
| if (typeof schema === 'object' && schema.properties && typeof value === 'object') { |
There was a problem hiding this comment.
@lukad / @dragidavid can we adopt the "early return" pattern anywhere we can? In this case it would be:
if (typeof schema !== 'object' || !schema.properties || typeof value !== 'object') {
return []
}
// the actual code...There was a problem hiding this comment.
I love early return! It makes the code much easier to read.
Also we have concluded in internal discussion that early return would use a safe fallback (e.g., empty array for errors) instead of fail-fast errors (e.g., throw) (Slack).
|
|
||
| export function createHeadlessForm(schema: JsfSchema, options: CreateHeadlessFormOptions = {}): FormResult { | ||
| const errors = validateSchema(options.initialValues, schema) | ||
| const validationResult = validationErrorsToFormErrors(errors) |
There was a problem hiding this comment.
praise: Thanks for doing this, following the official spec of json schema errors, and turn them into "formErrors" ✨
|
|
||
| if (getSchemaType(schema) === 'string' && typeof value === 'string') { | ||
| if (schema.minLength !== undefined && value.length < schema.minLength) { | ||
| errors.push({ path: [], validation: 'minLength', message: 'must be at least 3 characters' }) |
There was a problem hiding this comment.
🐛 Question: Why is "3 characters hardcoded"? 🤔
| errors.push({ | ||
| path: [], | ||
| validation: 'pattern', | ||
| message: `must match the pattern '${schema.pattern}'`, |
There was a problem hiding this comment.
For UX reasons, we improve this message even more v0 source. Must have a valid format. E.g. ${randomPlaceholder}
| import { createHeadlessForm } from '../../src' | ||
|
|
||
| describe('object schema validation', () => { | ||
| it('returns an error if the value is not an object', () => { |
There was a problem hiding this comment.
nit: Could we keep the same naming convention as v0 tests. Instead of saying if, saying when or given
| expect(form.handleValidation({})).not.toHaveProperty('formErrors') | ||
| expect(form.handleValidation({ address: {} })).not.toHaveProperty('formErrors') | ||
| expect(form.handleValidation({ address: 'not an object' })).toMatchObject({ | ||
| formErrors: { '.address': 'should be object' }, |
There was a problem hiding this comment.
bug/question: Why does the path has a . prefix. This is different from v0 and would break the internal code.
|
|
||
| expect(form.handleValidation({ address: { street: 'some street' } })).not.toHaveProperty('formErrors') | ||
| expect(form.handleValidation({ address: { street: 10 } })).toMatchObject({ | ||
| formErrors: { '.address.street': 'should be string' }, |
There was a problem hiding this comment.
bug/question: @lukad / @dragidavid do you realize that these error messages are different from v0? This will fail all tests. The error messages MUST be the same, otherwise a lot of tests (here and in internal codebases) will fail.
If we want to change the errors message, let that for later as an opt-in config or something.
There was a problem hiding this comment.
Yes, I mentioned this in another thread which I can't find now but after all we want all the error messages to be the same as before, definitely!
This implements string validation (minLength, maxLength, pattern) as well as the groundwork for validating objects.
I will address building fields, as well as
formErrorscompatibility with v0 in a follow up PR.