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

[FEATURE ds-boolean-transform-allow-null] allow null for boolean #4022

Merged
merged 1 commit into from Mar 18, 2016

Conversation

Projects
None yet
7 participants
@pangratz
Member

pangratz commented Dec 21, 2015

NOTE: this description is not completely accurate anymore, since the implementation changed so the allowNull flag needs to be passed to the options for the attribute. See discussion below for further context.


This feature allows null / undefined values for DS.attr('boolean')
attributes. Currently DS.BooleanTransform converts those values to
false; which means that a DS.attr('boolean') cannot be set to null
from the server. Other transforms (string, number) do allow null
values, so this is an inconsistency between the transforms.

The new feature ds-boolean-transform-allow-null changes this behavior
so null / undefined are converted to null. Since this is a semver
breaking change
, a corresponding deprecation warning should be added
before this is accepted in ember data.

Current behavior of DS.attr('boolean'):

incoming DS serialized
null false false
undefined false false

Behavior of DS.attr('boolean'), having
ds-boolean-transform-allow-null feature enabled:

incoming DS serialized
null null null
undefined null null

Note that this feature only works if ds-transform-pass-options is
enabled as well, since passing the options from DS.attr to the
transform is added with that feature.


This addresses #3785.


TODO

  • wait for implementation of emberjs/rfcs#1 so this PR can be updated with allowNull flag
@fivetanley

This comment has been minimized.

Show comment
Hide comment
@fivetanley

fivetanley Jan 5, 2016

Member

@pangratz During the team discussion someone suggested passing an options hash to DS.attr({allowNull: true}) so we could maintain backwards compatibility. What do you think about this change?

Member

fivetanley commented Jan 5, 2016

@pangratz During the team discussion someone suggested passing an options hash to DS.attr({allowNull: true}) so we could maintain backwards compatibility. What do you think about this change?

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jan 5, 2016

Member

Seems good to me. But for this to work emberjs/rfcs#1 (also see discussion in #3111) needs to be go'ed and implemented, since currently the hash is not passed to transformations.

Would the eventual default in 3.x be that null are allowed for DS.attr('boolean'), so it is consistent with number, string and date transforms?

Member

pangratz commented Jan 5, 2016

Seems good to me. But for this to work emberjs/rfcs#1 (also see discussion in #3111) needs to be go'ed and implemented, since currently the hash is not passed to transformations.

Would the eventual default in 3.x be that null are allowed for DS.attr('boolean'), so it is consistent with number, string and date transforms?

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jan 6, 2016

Member

Alright, emberjs/rfcs#1 has been accepted. Once that RFC is implemented I will update this PR so the allowNull flag is considered.

Member

pangratz commented Jan 6, 2016

Alright, emberjs/rfcs#1 has been accepted. Once that RFC is implemented I will update this PR so the allowNull flag is considered.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Feb 10, 2016

Member

Alrighty, I rebased onto latest master and now null is returned for null/undefined if the allowNull flag is set to true:

export default Model.extend({
  leBool: attr('boolean', { allowNull: true })
});
Member

pangratz commented Feb 10, 2016

Alrighty, I rebased onto latest master and now null is returned for null/undefined if the allowNull flag is set to true:

export default Model.extend({
  leBool: attr('boolean', { allowNull: true })
});
Show outdated Hide outdated addon/-private/transforms/boolean.js Outdated
[FEATURE ds-boolean-transform-allow-null] allow null for boolean
This feature allows `null` / `undefined` values for `DS.attr('boolean')`
attributes. Currently `DS.BooleanTransform` converts those values to
`false`; which means that a `DS.attr('boolean')` cannot be set to `null`
from the server. Other transforms (`string`, `number`) do allow `null`
values, so this is an inconsistency.

Current behavior of `DS.attr('boolean')`:

| incoming    | DS      | serialized |
|-------------|---------|------------|
| `null`      | `false` | `false`    |
| `undefined` | `false` | `false`    |

Behavior of `DS.attr('boolean')`, having
`ds-boolean-transform-allow-null` feature enabled and having an
attribute `DS.attr('boolean', { allowNull: true })`:

| incoming    | DS      | serialized |
|-------------|---------|------------|
| `null`      | `null`  | `null`     |
| `undefined` | `null`  | `null`     |

---

Note that this feature only works if `ds-transform-pass-options` is
enabled as well, since passing the options from `DS.attr` to the
transform is added with that feature.
@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Feb 17, 2016

Member

@bmac I somehow forgot to commit the changes considering the allowNull flag :facepalm: sorry for the jump start.

I - once again - updated the PR; now it should be ready for review 😉

Member

pangratz commented Feb 17, 2016

@bmac I somehow forgot to commit the changes considering the allowNull flag :facepalm: sorry for the jump start.

I - once again - updated the PR; now it should be ready for review 😉

@btecu

This comment has been minimized.

Show comment
Hide comment
@btecu

btecu Feb 17, 2016

Contributor

Wouldn't defaultValue work for this?
I think it should work at least for undefined.

Contributor

btecu commented Feb 17, 2016

Wouldn't defaultValue work for this?
I think it should work at least for undefined.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Feb 24, 2016

Member

I think this wouldn't work since defaultValue is only used when the value of an attribute is retrieved from a DS.Model, but not when an attribute is serialized...

Member

pangratz commented Feb 24, 2016

I think this wouldn't work since defaultValue is only used when the value of an attribute is retrieved from a DS.Model, but not when an attribute is serialized...

var type = typeof serialized;
if (isEnabled('ds-transform-pass-options')) {
if (isEnabled('ds-boolean-transform-allow-null')) {
if (isNone(serialized) && options.allowNull === true) {

This comment has been minimized.

@bmac

bmac Mar 18, 2016

Member

Should we use Ember.get(options, 'allowNull') in case options is undefined?

@bmac

bmac Mar 18, 2016

Member

Should we use Ember.get(options, 'allowNull') in case options is undefined?

This comment has been minimized.

@pangratz

pangratz Mar 18, 2016

Member

Theoretically options should never be undefined because of this and ultimately because of this.

It would only be undefined if the transform is used outside of regular ember-data usage...

@pangratz

pangratz Mar 18, 2016

Member

Theoretically options should never be undefined because of this and ultimately because of this.

It would only be undefined if the transform is used outside of regular ember-data usage...

This comment has been minimized.

@bmac

bmac Mar 18, 2016

Member

Sounds good.

@bmac

bmac Mar 18, 2016

Member

Sounds good.

bmac added a commit that referenced this pull request Mar 18, 2016

Merge pull request #4022 from pangratz/boolean-transform-allow-null
[FEATURE ds-boolean-transform-allow-null] allow null for boolean

@bmac bmac merged commit 24dd96f into emberjs:master Mar 18, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pangratz pangratz deleted the pangratz:boolean-transform-allow-null branch Mar 18, 2016

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Jul 26, 2016

Contributor

This is a great adjustment! 🎆

@bmac @fivetanley for 3.0, would you mind flipping the default of the option (or removing the option altogether)? In our app I'd say 99% of boolean values across all our models would use the allowNull option. Also, it's inconsistent with string, number etc. where no option is required.

Contributor

sandstrom commented Jul 26, 2016

This is a great adjustment! 🎆

@bmac @fivetanley for 3.0, would you mind flipping the default of the option (or removing the option altogether)? In our app I'd say 99% of boolean values across all our models would use the allowNull option. Also, it's inconsistent with string, number etc. where no option is required.

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