[BUGFIX beta] Allow numeric keys for the get keyword

[duggiefresh]

Fixes #13296

[duggiefresh]

Calling toString() here because of this assertion. Let me know if there's a better direction. Thanks!

[rwjblue]

Test should be added in https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/tests/integration/helpers/get-test.js (using that new format). That file is symlinked to ember-htmlbars tests so that it runs under both glimmer and htmlbars rendering engines.

See #13127 for a detailed description of that test format (and the I-N-U-R test cycle we are using in the ember-glimmer test suite).

[duggiefresh]

Oh yes! Thanks 💚

[duggiefresh]

Updated per review. I made an update to Glimmer util/references that will "stringify" numeric keys.

Thanks!

[chancancode]

Can you add a test for this? (If the browser supports it?)

[chancancode]

if (supportsNativeSymbols) {
  moduleFor('Helpers test: {{get}} with symbols', class extends RenderingTest {
    // ...
  });
}

[duggiefresh]

Can do! Is there an example somewhere of how to check for browser support within tests? Thanks

[chancancode]

I think something like what I mentioned above should work:

moduleFor('Helpers test: {{get}}', class extends RenderingTest {
  // current tests
}

if (typeof Symbol === 'function') { // I think?

moduleFor('Helpers test: {{get}} with symbols', class extends RenderingTest {
  // moar tests
});

}

[duggiefresh]

Thanks for the help. However, it seems there isn't support for symbols in templates. Do we want to add symbol support for the {{get}} keyword?

For example, something like this doesn't work either:

['@test should be able to get an object value with numeric keys']() {
  let first = Symbol('first');
  let second = Symbol('second');
  let third = Symbol('third');

  this.render(`{{#each indexes as |index|}}[{{get items index}}]{{/each}}`, {
    indexes: [first, second, third],
    items: {
      [first]: 'First',
      [second]: 'Second',
      [third]: 'Third'
    }
  });

°°°

[rwjblue]

No, if we don't support symbols already in template-land, then we don't need to support it explicitly here.

[duggiefresh]

Great! Thanks will update the PR as necessary

[duggiefresh]

Sorry to bother, but just a ping to move this along.

Please see: #13311 (comment)

Thanks, @chancancode @rwjblue

[krisselden]

I don't understand why we don't check for number and coerce it? Can we start conservatively? Does it make sense to toString anything? This will toString null to 'null' for example.

[krisselden]

Especially if this is targeted as a bugfix to beta.

[mixonic]

@duggiefresh I think there was interest in removing the symbol specific code here? /cc @rwjblue

And @krisselden's suggestion re: swapping to a conservative approach, only coercing numbers, seems reasonable.

Would love to see this land :-)

[duggiefresh]

@mixonic Thanks for pinging me on this. I believe I'll be attending your Contributors Workshop at WGE. Perhaps, we can land this before the conference. :)

See you then

[duggiefresh]

Updated per comments! I removed the symbol references and am now coercing number keys. Thanks 🍨

[duggiefresh]

ping @mixonic @krisselden Thank you!

[Turbo87]

does this work for 0 as a key too? I know I can use firstObject too, but in a index-loop it would be easier if 0 would work too.

[mixonic]

@Turbo87 I'm not quite sure what you're asking. Can you provide an example?

[mixonic]

I kind of expect you would only need the logic in the keyword. What other use-cases does this cover?

[duggiefresh]

I added the coercion here too, because of the assertion here: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/property_get.js#L49

[Turbo87]

@mixonic I'm talking about something like

{{#each someArray as |value index|}}
  value: {{get some.other.array index}}
{{/each}}

[krisselden]

can you explain why it is needed to be check in both places?

[duggiefresh]

I added a coercion in both ember-glimmer and ember-htmlbars because of the assertion here: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/property_get.js#L49

Perhaps there's a better place to add this logic?

[homu]

☔ The latest upstream changes (presumably #14041) made this pull request unmergeable. Please resolve the merge conflicts.

[locks]

@duggiefresh are you able to rebase and solve the conflicts? Thank you!

[tsteuwer]

Pease let this happen. I have to manually change keys to strings to use get with them 😊

[rwjblue]

I rebased and updated to work on master, but this PR predates github's feature that allows project maintainers to push to PR branches. I resubmitted (with correct commit attribution) in #15366.