getWithDefault broken with chained properties

[jamesarosen]

Let's explore how get(...) || y and getWithDefault(x, y) compare.

> o = Ember.Object.create()
> o.get('foo') || 'x'
"x"
> o.getWithDefault('foo', 'x')
"x"

👍 OK, that sounds totally right.

> o.set('foo', 'bar')
> o.get('foo') || 'x'
"bar"
> o.getWithDefault('foo', 'x')
"bar"

👍 Cool. Makes sense.

> o = Ember.Object.extend({ foo: null }).create()
> o.get('foo') || 'x'
"x"
> o.getWithDefault('foo', 'x')
null

😕 Hmm. That obeys the API docs but it doesn't seem super intuitive. Others agree with me, but there's also good reason to keep it as-is. In our app, we use foo: null as an indication that the client should pass foo in; maybe we should change that to foo: undefined. Oh well, let's press on.

> o = Ember.Object.create()
> o.get('foo.bar')
undefined
> o.getWithDefault('foo.bar', 'x')
"x"

👍 Diggin' it.

> o = Ember.Object.extend({ foo: null }).create()
> o.get('foo.bar') || 'x'
"x"
> o.getWithDefault('foo.bar', 'x')
null

WHAT!?!?!?!??!!?

[mmun]

This is a bug in Ember.get:

Ember.get({ foo: null }, 'foo.bar') // null

It should be undefined.

[jamesarosen]

@mmun on Ember 1.13, I get

> Ember.get({ foo: null }, 'bar')
undefined

which is correct as far as I can tell.

[mmun]

Oops. I was checking using the console on http://discuss.emberjs.com/ but it seems to be running 1.12 :P

[mmun]

You want foo.bar there, btw.

[mmun]

I just checked it against canary and Ember.get({ foo: null }, 'foo.bar') === null still.

[jamesarosen]

You want foo.bar there, btw.

Ah!

[pittst3r]

I have a failing test for this. I'll start a WIP PR.

[jamesarosen]

Here is the bad line as far as I can tell. My guess is that it should be something more like

if (obj == null) {
  return i === parts.length - 1 ? obj : undefined;
}

[locks]

related: #13449

[pittst3r]

@jamesarosen Yeah, that's where I ended up as well. This was my solution:

if (obj == null) {
  if (i < parts.length) {
    return undefined;
  }
  return obj;
}

[pittst3r]

I haven't committed it yet though because I'm not convinced it's the best solution. Going to keep tinkering with this.

[pittst3r]

Tried a recursive approach for _getPath but it didn't turn out to be the degree of improvement I had hoped for so I'm sticking with what's there already, but with the above fix. The PR is ready unless anyone has another suggestion. 💃

[pittst3r]

@jamesarosen Looks like the fix will be in the next beta.

This issue can be closed now.