-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update isemail from 2.x.x to 3.x.x #1229
Conversation
@@ -561,7 +551,8 @@ describe('string', () => { | |||
Helper.validate(schema, [ | |||
['joe@example.com', true], | |||
['joe@www.example.com', true], | |||
['joe@localhost', false, null, '"value" must be a valid email'], | |||
['joe@localhost', true], | |||
['joe@', false, null, '"value" must be a valid email'], |
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.
Should we add a test here for a unicode address? êjness@something.com
for example. We don't want to ensure the operation of isemail
but I think it would be good to make sure we continue to support unicode going forward and throughout the Joi part of the code base.
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.
Sure :)
test/types/string.js
Outdated
@@ -414,16 +414,6 @@ describe('string', () => { | |||
done(); | |||
}); | |||
|
|||
it('throws when checkDNS option is enabled', (done) => { |
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.
No harm in keeping this test.
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.
Alright
@@ -561,7 +551,8 @@ describe('string', () => { | |||
Helper.validate(schema, [ | |||
['joe@example.com', true], | |||
['joe@www.example.com', true], | |||
['joe@localhost', false, null, '"value" must be a valid email'], |
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.
Is this an expected breaking change ? /cc @skeggse
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 believe so, extrapolating from the resolution of skeggse/isemail#154
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.
Assuming that test used to specify that joe@localhost
was not a valid email, yes, this is an expected change. I'd actually like to see the same test, but true
.
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.
Makes me wonder if this should be a breaking change for joi or if it's considered a bugfix from your pov.
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'd actually like to see the same test, but true
I'm not sure why the Github UI is showing that line just taken out. I didn't remove it, I did just change the assertion to true
:)
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.
It seems like a bugfix to me - users who do not desire this behavior can add localhost
to their tldBlacklist
.
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
isemail
v3.0.0 has been released, adding support for unicode emails and removing DNS lookups entirely.