Skip to content

Handle undefined properties#175

Open
noirbizarre wants to merge 1 commit into
geraintluff:masterfrom
noirbizarre:undefined
Open

Handle undefined properties#175
noirbizarre wants to merge 1 commit into
geraintluff:masterfrom
noirbizarre:undefined

Conversation

@noirbizarre

Copy link
Copy Markdown
Contributor

This pull-request allow to validate objects with properties set to undefined if the properties are not required.

It seems coherent with this:

JSON.stringify({nullProp: null, undefinedProp: undefined})
>> {"nullProp":null}"

So with this schema:

var schema = {
    properties: {
        key: {type: 'string'}
    }
};

we should have:

tv4.validate({key: undefined}, schema);
>> true
tv4.validate({key: null}, schema);
>> false

@noirbizarre

Copy link
Copy Markdown
Contributor Author

Hi!
Any comment on this one ?
Is it possible to merge it ?

@geraintluff

Copy link
Copy Markdown
Owner

Sorry for the delay.

I like this, but it may represent a subtle change in behaviour. I'm trying to figure out whether it will screw up anybody's existing validation.

The spec's understandably silent on undefined values, so I think it might be safe, though. This is probably good to go in. (@Bartvds, any objection?)

@noirbizarre

Copy link
Copy Markdown
Contributor Author

No worries, it was only to make sure you've seen it ;)

I agree, it introduce a very subtle change, but as you said, it was not specified.
I think this is because undefined is a JavaScript only concept and does not exists in JSON (which explain the JSON.stringify behavior), so it's not defined in JSON Schema (which is consistent).

@klyngbaek

Copy link
Copy Markdown

+1
Any chance we merge this in?

@klyngbaek

Copy link
Copy Markdown

This is actually the only thing keeping me from using this module :)

@noirbizarre

Copy link
Copy Markdown
Contributor Author

Hi!
Any news on this one ?

@polpo

polpo commented Oct 23, 2015

Copy link
Copy Markdown

I just made a PR for this same issue without noticing this one. Seems like this is a fix that a lot of people need!

@noirbizarre

Copy link
Copy Markdown
Contributor Author

Yes, this is the third PR on the topic.

I think we are just waiting for @Bartvds to confirm that it can be merged.

@drom

drom commented Apr 28, 2017

Copy link
Copy Markdown

Any progress on the merge of PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants