[BUGFIX beta] Negative indices in positional args proxy

[buschtoens]

In ember-link I had code that potentially accessed a negative index on a positional args proxy, which used to return undefined. Starting with the 3.23.0 beta, this throws an error.

This is because only the upper bound is checked.

[rwjblue]

I think it may be better to change convertToInt to return null for negative indices.

@pzuraq / @krisselden - What do you think?

[rwjblue]

I don't think this fix is quite enough (we have the exact same type of check in has).

[rwjblue]

Also, we need a test for this behavior (confirming that modifiers,helpers, and components can access args[-1] and get undefined back).

[buschtoens]

Cool! Let me know on the way forward regarding:

I think it may be better to change convertToInt to return null for negative indices.

I'll update the PR then and add tests. 🧪

[rwjblue]

Starting with the 3.23.0 beta, this throws an error.

Oh, also, what is the error here?

[buschtoens]

Uncaught (in promise) TypeError: ref is undefined
    valueForRef reference.js:145
    get
    normalizeLinkParams
    compute
    getValue
    ...

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

Lines 104 to 106 in 0f3266c

	if (parsed !== null && parsed < positional.length) {
	return valueForRef(positional[parsed]);
	}

https://github.com/glimmerjs/glimmer-vm/blob/022b586405a857afdf59eebfcdae1f51428dc344/packages/%40glimmer/reference/lib/reference.ts#L153

ref is undefined as positional[-1] is undefined.

[rwjblue]

Cool! Let me know on the way forward regarding:

I think it may be better to change convertToInt to return null for negative indices.

Just chatted with @pzuraq / @krisselden about this, lets do it this way. I think that would look something like:

function convertToInt(prop: number | string | symbol): number | null {
  if (typeof prop === 'symbol') return null;

  const num = Number(prop);

  if (isNaN(num)) return null;

  return num % 1 === 0 && num > -1 ? num : null;
}

Then we just need some tests, and to update the commit message/PR title to [BUGFIX release] since 3.23 has been released.

[kategengler]

Is this still relevant?