each-in breaks on keys that contain a period in 3.8.0-beta.1

[jasonmit]

{{each-in}} will return undefined for the value whose key contains a period.

This bug does not exist in 3.7.2 and first appears in 3.8.0-beta.1 and is still present in 3.8.0-beta.3.

Fail test (drop in packages/@ember/-internals/glimmer/tests/integration/syntax/each-in-test.js)

[`@only each-in supports keys with a period in them`]() {
  this.render(
    strip`
    <ul>
      {{#each-in categories as |_ item|}}
        <li>{{item.name}}</li>
      {{/each-in}}
    </ul>
  `,
    {
      categories: {
        // uncomment and run.  notice `items` is undefined
        'hello.world': { name: 'foo' },
        // uncomment and run.  notice it works as expected
        // hello world: { name: 'foo' },
      },
    }
  );

  // Empty
  this.assertHTML(strip`
    <ul>
      <li>foo</li>
    </ul>
  `);
}

[locks]

@jasonmit could you send a PR with the failing test?

[rwjblue]

We should definitely root cause this, but I'm not sure that this will be considered a bug in the end. Specifically, get(obj, 'hello.world') will look for obj.hello.world not obj['hello.world']...

[chancancode]

Yeah pretty sure that’s the reason, each-in uses get so this is sort of a duplicate of the get with dot path ticket. Mayyyybe in this specific case we don’t need to use get in here after landing ES5 getters because this probably doesn’t work with proxies anyway?

[chancancode]

Specifically https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/glimmer/lib/utils/iterable.ts#L128

I agree I’m not sure if this is considered a bug and whether fixing this now will cause problems elsewhere or down the road

[rwjblue]

Ya, and I actually think we do different things based on the heuristics on the object being iterated here:

ember.js/packages/@ember/-internals/glimmer/lib/utils/iterable.ts

Lines 290 to 298 in 7df81ec

	if (Array.isArray(iterable) || isEmberArray(iterable)) {
	return ObjectIterator.fromIndexable(iterable, this.keyFor(true));
	} else if (HAS_NATIVE_SYMBOL && isNativeIterable<[Opaque, Opaque]>(iterable)) {
	return MapLikeNativeIterator.from(iterable, this.keyFor());
	} else if (hasForEach(iterable)) {
	return ObjectIterator.fromForEachable(iterable, this.keyFor());
	} else {
	return ObjectIterator.fromIndexable(iterable, this.keyFor(true));
	}

Some of those iterators use get (ObjectIterator.fromIndexable) and some don't (ObjectIterator.fromForEachable, MapLikeNativeIterator.from)

[chancancode]

We are looking at the fromIndexable in this case. I think the only problem with not using get is pojos with cps

[rwjblue]

@chancancode - ya, I only point it out because @jasonmit could possibly work around the issue by making the object implement Symbol.iterable

[jasonmit]

@jasonmit could you send a PR with the failing test?

Definitely, I'll do that now and link the issue.

ya, I only point it out because @jasonmit could possibly work around the issue by making the object implement Symbol.iterable

Great idea, I'll try that out as a work around.

In case you're curious why we have periods in the key, the keys in the object being iterated over are ISO strings (i.e., 2011-10-05T14:48:00.000Z) and those have periods before the UTC offset.

[amk221]

Just wondering what the status of this is? Thanks all

[jasonmit]

@amk221 I don't believe this was resolved, we ended up refactoring our app to avoid the period in the key but obviously not an ideal solution for everyone.

[eplata31]

Hi everyone, I made a PR for this one, asking for a review :D

[amk221]

Anything I can do to get this moving along? We really need this 🤞

[AlexAndBear]

@amk221 you can add a helper. Thats how I solved the Problem.
But agree this is critical.

[iagox86]

Just wasted a couple hours on this - I'm super new to Ember and this was completely surprising.

For my app, I'm tracking status for a bunch of hosts, so keying a hash by the IP or domain name. It was returning undefined for everything except localhost and I was driving myself crazy trying to figure this out.

[amk221]

@devop911 care to share your helper 😁

[iagox86]

@amk221 Not the one you're asking, but I ended up using this a bunch:

import { helper } from '@ember/component/helper';
import _ from 'lodash';

export function hashToArray([h]) {
  return _.map(h, function(value, key) {
    return {
      key: key,
      value: value,
    }
  });
}

export default helper(hashToArray);

[RobbieTheWagner]

I just hit this issue as well. Any updates?

[pzuraq]

I'm not sure we can fix this without regressing the common use case for get. If we can get a benchmark that shows the regression isn't bad, then it's probably fine to add this to get, but if not then we would need to build the fix directly into each-in. I suspect it would be a decent regression.

We could manually check for proxies and use unknownProperty on the object if it was a proxy, and otherwise use normal get syntax specifically when iterating an object using each-in, that may work as expected.

Long term, tracked props can't come soon enough 😩

[AlexAndBear]

Long Story short: Switch to anything else then Ember. It's not worth all the trouble.

[pzuraq]

@devop911 comments like that aren't super constructive or helpful. We absolutely want to solve this problem, but we also don't want to regress performance in user's applications for an edge case that is relatively uncommon. It's a difficult situation to be in as a maintainer.

We'll figure out a fix for this in time, it just needs a bit more work than a typical issue and we've been focused on shipping features for Octane. I agree, this is important though.

[AlexAndBear]

@pzuraq
Let the users decide, how to name their keys, can never be an edge case.

[RobbieTheWagner]

@pzuraq I think we should at least have a warning that is thrown if the key to each-in includes a period. There isn't a great solution without introducing regressions though, you are right

[pzuraq]

@devop911 my assessment of it being an edge case was based on the fact that we've seen very few reports of this bug overall since this issue was opened. If this were behavior that every Ember application relied on, it would have been a higher priority fix, absolutely, but it seems that not many folks use periods in their keys.

On the flip side, we do get lots of complaints whenever we regress performance. If this weren't a hot path, the fix would definitely have come a lot earlier. Like I said, it's a difficult situation to be in as a maintainer, especially when you're trying to get things done 😩 I'll see what we can do to get someone working on this sooner rather than later

@rwwagner90 I do think we can fix the behavior, so I'd rather not start warning, but if we can't fix it I agree we should definitely warn users.

[rwjblue]

Fixed by #18296

[jasonmit]

Thanks everyone for coming up with a solve ❤️