-
Notifications
You must be signed in to change notification settings - Fork 18
Correct validator to use rfc822 email validation #1135
Conversation
aaf28ee
to
d80f623
Compare
Fixed the tests here. Ready for review. |
static_src/util/validators.js
Outdated
@@ -41,7 +41,7 @@ export function validateString() { | |||
|
|||
export function validateEmail() { | |||
return function _validateEmail(value, name) { | |||
if (!(/^\w+([\.-]?\w+)*@\w+([\.-]?\w+)*(\.\w{2,3})+$/.test(value))) { | |||
if (!(/^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$/).test(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might make the regex itself a const
outside the function so we don't have to create it each time we call validateEmail
. As a bonus it can replace the regexp literal with a readable name!
@el-mapache Great point. I just stored this as a variable. |
static_src/util/validators.js
Outdated
@@ -41,7 +41,8 @@ export function validateString() { | |||
|
|||
export function validateEmail() { | |||
return function _validateEmail(value, name) { | |||
if (!(/^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$/).test(value)) { | |||
const emailRegex = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this outside the exported function, this will still re-parse the regex every time the function is called. Its not a big deal performance-wise but I think its good practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@el-mapache Just moved up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.