Skip to content
This repository has been archived by the owner on Feb 16, 2021. It is now read-only.

t.maybe setting the value to null when missing #183

Closed
gabro opened this issue Feb 19, 2016 · 12 comments
Closed

t.maybe setting the value to null when missing #183

gabro opened this issue Feb 19, 2016 · 12 comments

Comments

@gabro
Copy link
Contributor

gabro commented Feb 19, 2016

Is there a reason why t.maybe sets the value to null when missing?

This behavior turns a missing value (e.g. undefined) into a value (null) and it looks like a potentially undesirable side-effect.

For instance this causes issues with tcomb-react: since null is a valid value, React will not use the value provided by defaultProps.

@props({
  foo: t.maybe(String)
})
class MyComponent extends React.Component {

  static defaultProps = {
    foo: 'bar'
  }

  ...
}

<MyComponent /> // Invalid value null supplied to foo

A more natural implementation would look like

return Nil.is(value) ? value : create(type, value, path);

Do you think that's reasonable, @gcanti?

@gcanti
Copy link
Owner

gcanti commented Feb 19, 2016

Well, this is a very opinionated part of tcomb: my goal was to get rid of undefined and null and unify them.

Coming to your use case there's something I don't get. Since you are providing a default value, de facto foo is a required string, thus the correct typing would be:

@props({
  foo: t.String // <= foo is always a string
})
class MyComponent extends React.Component {

  static defaultProps = {
    foo: 'bar'
  }

  ...
}

Now @props doesn't alter the value passed in, just validates the input. Hence my guess is that you are passing null to the foo prop from outside. If that's the case, even with your change, you won't see the expected result.
So maybe the culprit is a maybe outside MyComponent?

@gabro
Copy link
Contributor Author

gabro commented Feb 19, 2016

my goal was to get rid of undefined and null and unify them.

that'd be great in theory, but it clashes with the cruel reality. Javascript works differently, unfortunately, and many other libraries tcomb interoperates with will make the undefined !== null assumption.
Even ES6 destructuring distinguishes between the two cases when assigning default values.

const { foo = 'bar', baz = 'hey' } = { foo: null } 
console.log(foo) // null
console.log(baz) // hey

thus the correct typing would be

@props({
  foo: t.String // <= foo is always a string
})

Not sure I agree. props represents the api of my component and - from the outside - foo is optional.
Considering that we use props to document the component, it feels more natural to declare it as optional.

Now @props doesn't alter the value passed in, just validates the input.

True, in theory, but doesn't the process of validating makes the values to go through the maybe constructor. The validation in the example above fails because undefined (prop was not passed) is turned into null. Or am I mistaken?

@gcanti
Copy link
Owner

gcanti commented Feb 19, 2016

Considering that we use props to document the component, it feels more natural to declare it as optional

I guess it's a matter of taste, such a documentation should also report the presence of a default. That typing is also safer when you refactor the code (say you mistakenly delete defaultProps...). But I digress, t.maybe(t.String) is fine, I agree.

True, in theory, but doesn't the process of validating makes the values to go through the maybe constructor

AFAIK no, it doesn't. As a proof scan the tcomb-react's code or just run your own example:

import React from 'react'
import ReactDOM from 'react-dom'
import { t, props } from 'tcomb-react'

@props({
  foo: t.maybe(t.String)
})
class MyComponent extends React.Component {

  static defaultProps = {
    foo: 'bar'
  }

  render() {
    return <div>Hello {this.props.foo}</div>
  }
}

ReactDOM.render(<MyComponent />, document.getElementById('app'))

Output

<div data-reactid=".0">
  <span data-reactid=".0.0">Hello </span>
  <span data-reactid=".0.1">bar</span>
</div>

Let's make clear the rationale and the assumptions of this library: tcomb is grounded on math, and specifically on set theory. In JavaScript we often need to express that a value is optional, but how to represent the "absence of a value" in set theory? The empty set is not a good candidate as it contains no elements. So we need to define a conventional set Nil whose elements represent the "absence of a value". Ideally Nil should contain only one element (undefined) but what about null? Doesn't null convey the same concept? the "absence of a value"? So Nil contains both undefined and null.

This is perhaps the most opinionated choice in tcomb (actually I'm a little surprised that nobody complained about this choice before so thank you for this discussion).

How to represent an "optional string"? The tcomb answer is: an optional string is the set union of two sets: the t.Nil set and the set of all strings (t.String) leading to the set t.maybe(t.String).

Now how to represent "default values"? A natural answer would be: given a function f : A -> B and an element a in A one can build a new function f' : A U Nil -> B such that:

f'(x) = f(x) if x in A
f'(x) = f(a) if x in Nil

tl;dr

Personally I think that having both null and undefined is a big mistake, and I look at default values with suspicion but I'm not interested in holy wars. If the current implementation clashes with the JavaScript ecosystem and if the change you are proposing is backward compatible I'm willing to adopt it right away.

@gabro
Copy link
Contributor Author

gabro commented Feb 19, 2016

Thank you for the thorough explanation. Regarding the tcomb-react specific issue I'll investigate further. I may be mistaken.

For the theoretical-side of the conversation, you raise an excellent point

Ideally Nil should contain only one element [...]

but in Javascript

Nil contains both undefined and null.

tcomb can be (and it is) mathematically sound internally. Laws are defined in terms of the Nil set, but the JS representation of a Nil set is the union of undefined and null.

So, when you define a function f' : A U Nil -> B

f'(x) = f(x) if x in A
f'(x) = f(a) if x in Nil

I would expect

f'(null) = f'(undefined)

Unfortunately, the definition of such function is often beyond the scope of tcomb.
React has its own definition for prop types and ES6 has its own definition for de-structuring and default parameters. Both look something like

f'' : A U [undefined] -> B

f''(x) = f(x) if x in A
f''(x) = f(a) if x in [undefined]

Given f'' is what we usually get in the JS ecosystem, I would prefer tcomb to be agnostic in the JS representation of the Nil set. Both undefined and null belong to it in tcomb, but I think the actual underlying representation shouldn't be chosen arbitrarily by the library.

@gcanti
Copy link
Owner

gcanti commented Feb 19, 2016

but I think the actual underlying representation shouldn't be chosen arbitrarily by the library

I agree. The main question now is: would this change be considered a breaking change?

@gabro
Copy link
Contributor Author

gabro commented Feb 19, 2016

Yes, I think so.

There's a chance somebody out there is relying on

maybe(undefined) = null

and it would change to

maybe(undefined) = undefined

Given that undefined != null, this is a breaking change, in my opinion.

@jaxyeh
Copy link

jaxyeh commented Mar 7, 2016

@gcanti - Any updates or thoughts on that front?

I've started to use tcomb library for this particular server-side code (node.js), it allows me to struct & validate a model before passing through a database query. I've encountered same dilemma as @gabro, where the database query would consider null as NULL value on an insert method. However, it would've ignored if they were undefined values.

Or at least temporary, could you recommend a code/method to filter/reverse those null values in a tcomb-based struct object?

For now, I'm using this prototypal extension as a workaround:

'use strict';
const _ = require('lodash');

const Person = t.struct({
  id: t.maybe(t.String),
}, 'Person');

Person.prototype.toJSON = function () {
  let purge = (current) => {
    _.forOwn(current, (value, key) => {
      if (_.isUndefined(value) || _.isNull(value)) {
        delete current[key];
      }
    });
    if (_.isArray(current)) _.pull(current, undefined);
    return current;
  };
  return purge(_.cloneDeep(this));  // Do not modify the original object, create a clone
};

@volkanunsal
Copy link

Relevant to this discussion. Douglas Crockford on null vs undefined:

https://youtu.be/rhV6hlL_wMc?t=1326

@gcanti
Copy link
Owner

gcanti commented Mar 7, 2016

@jaxyeh thanks for reporting. I think that the only reasonable solution is to go ahead with a breaking change and to release a v3.0.0.
It's kind of unfortunate since it will affect all the ecosystem but I try to strictly follow semver and I don't want to risk unexpected behaviors for the users. I'll add a note in the changelog explaining that if you don't rely on the property maybe(MyType)(undefined) = null this is not a breaking change for you.

@gabro Would you like to send a PR for this?

@gabro
Copy link
Contributor Author

gabro commented Mar 7, 2016

It's on its way

@gcanti gcanti closed this as completed in c8dea3b Mar 7, 2016
gcanti added a commit that referenced this issue Mar 7, 2016
Prevent Maybe constructor from altering the value when Nil (fixes #183)
@gcanti
Copy link
Owner

gcanti commented Mar 7, 2016

@gabro
Copy link
Contributor Author

gabro commented Mar 7, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants