Conversation
…or idn-email format (leveraging ajv-formats) Signed-off-by: Pasi Eronen <pe@iki.fi>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
Documentation build overview
Show files changed (1 files in total): 📝 1 modified | ➕ 0 added | ➖ 0 deleted
|
jkowalleck
left a comment
jkowalleck
left a comment
There was a problem hiding this comment.
Thank you so much for taking care of this topic.
see my remarks below
| ajv.addFormat('iri-reference', true) | ||
|
|
||
| // add idn-email format (was previously provided by ajv-formats-draft2019) | ||
| const emailValidator = ajv.compile({type: 'string', format: 'email'}) |
There was a problem hiding this comment.
since using our own implementations, we critically need tests for this!
please add test cases:
- positive cases - like "this is expected to be treated as a valid 'idn-email'"
- negative cases - like "this is expected to be treated as an invalid 'idn-email'"
- edge cases - like UTF8 characters, quoted emails (
"foo@bar"@baz) and waht not in the
There was a problem hiding this comment.
For anything containing only ASCII characters, we just reuse the implementation from ajv-formats (whose definition of what exactly is a valid email might not be identical to somebody else's definition - I don't think we want to write detailed tests for that).
But I agree we need something here to show the code works. I took the test cases I found in https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/e82bfdfa59c63cc74175d35fac81bb95e61db24b/tests/draft2019-09/optional/format/email.json and https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/e82bfdfa59c63cc74175d35fac81bb95e61db24b/tests/draft2019-09/optional/format/idn-email.json,
would those be enough?
There was a problem hiding this comment.
could you also add edge cases?
Things i can imagine:
- empty email address (empty string) - shall be a negative
- to be continued
here are other cases that might be woven in:
see also the test cases of the already used library for reference:
There was a problem hiding this comment.
Sure! Especially the empty string is a good idea, added couple more at the same time (amended the same commit).
I also noticed that not even ajv-formats and ajv-formats-draft2019 agree on every input. For example, "John Doe"@example.com is valid according to the latter but not the former. That said, I don't expect this to be a problem (and we probably don't want too many tests for rare edge cases, since ajv-formats itself may also change over time).
Signed-off-by: Pasi Eronen <pe@iki.fi>
pasieronen
force-pushed
the
replace-ajv-formats-draft2019
branch
from
3779054 to
6f127bd
Compare
2 participants
Description
Replace unmaintained dependency ajv-formats-draft2019 with our own implementation.
Continues work started by grease-work-23 in #1226. See also discussion in #1223 and
CycloneDX/cyclonedx-webpack-plugin#1348
The only piece we still used from ajv-formats-draft2019 is "idn-email". I tried to keep the code simple, and re-use ajv-formats (which we anyway depend on) as much as possible.
It might be possible to implement "iri-reference" in similar way (convert to URI, and use "uri-reference" from ajv-fomats), but I'm not sure how useful that would be: almost any string is a valid "uri-reference", so it wouldn't probably catch any problems anyway.
Resolves or fixes issue: #1226
AI Tool Disclosure
Affirmation