Skip to content

allow null setting for domain#65

Merged
jaw187 merged 4 commits intohapijs:masterfrom
mshick:master
May 4, 2015
Merged

allow null setting for domain#65
jaw187 merged 4 commits intohapijs:masterfrom
mshick:master

Conversation

@mshick
Copy link
Copy Markdown
Contributor

@mshick mshick commented Apr 20, 2015

Per #64 allows domains to be set to null an allowed value for server.state.

@hueniverse hueniverse added the feature New functionality or improvement label Apr 20, 2015
@gansbrest
Copy link
Copy Markdown

yep, +1 )

lib/index.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Statehood sets domain to null via defaults, this isn't needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense. It's worth pointing out though, that with the validation happening first there's no way to trigger that branch of logic -- no falsey value will pass validation for domain, presently.

@jaw187
Copy link
Copy Markdown
Contributor

jaw187 commented Apr 30, 2015

@mshick include changes for redirectTo as well and any other property which you need to send falsey values for.

@jaw187 jaw187 added this to the 2.1.1 milestone Apr 30, 2015
@jaw187
Copy link
Copy Markdown
Contributor

jaw187 commented Apr 30, 2015

@gansbrest Do you have an specific property and falsey value which you need support for?

@mshick
Copy link
Copy Markdown
Contributor Author

mshick commented Apr 30, 2015

Okay, will do. I’ll push up the validation change in a sec, along with a test.

I don’t mean to badger you. If redirectTo isn’t a change you want to make I understand.

On Apr 30, 2015, at 10:04 AM, James Weston notifications@github.com wrote:

@mshick https://github.com/mshick include changes for redirectTo as well and any other property which you need to send falsey values for.


Reply to this email directly or view it on GitHub #65 (comment).

@hofan41
Copy link
Copy Markdown
Contributor

hofan41 commented Apr 30, 2015

Should domain also be changed to allow false?

@mshick
Copy link
Copy Markdown
Contributor Author

mshick commented Apr 30, 2015

Null is allowed by statehood, false is not. That's the rationale -- consistency.

On Apr 30, 2015, at 1:49 PM, Ho-Fan Kang notifications@github.com wrote:

Should domain also be changed to allow false?


Reply to this email directly or view it on GitHub.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be allow('')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used false to be consistent with the docs for redirectTo at the route level:

 To set an individual route to use or disable redirections, use the route plugins config ({ config: { plugins: { 'hapi-auth-cookie': { redirectTo: false } } } }). Defaults to no redirection.

In light of that, would you still like me to change it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the allow value will override the data type.

> Joi.validate(false, Joi.string().allow(false))
{ error: null, value: false }

jaw187 added a commit that referenced this pull request May 4, 2015
allow null setting for domain
@jaw187 jaw187 merged commit bc76bb4 into hapijs:master May 4, 2015
@lock
Copy link
Copy Markdown

lock bot commented Jan 9, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature New functionality or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants