[BUGFIX release] Ensure hash properties are reified lazily

[pzuraq]

(hash) previously had two different types of behavior, one where it
was used as a reference and could get individual properties lazily, and
one where it was fully reified and exposed in JS. The lazy nature of
hash was very useful as it prevented having to realize the full cost of
each reference immediately, and it also meant that some values could
be accessed in conditionals and run assertions just in time. For
example, if someone was using a component that was loaded lazily via
an engine, you could render the component on a hash within a conditional
and it would not cause an error (because the component did not exist):

<SomeComponent as |api|>
  <api.eagerComponent/>

  {{#if this.displayLazy}}
    <api.lazyComponent/>
  {{/if}}
</SomeComponent>

This PR restores this ability by having (hash) return a proxy rather
than an object, which allows us to access each individual value without
reifying the references immediately. For backwards compatibility, we
also allow users to set values on the proxy, so long as they are not
setting any of the values defined on the proxy initially, since those
are read-only references.

[simonihmig]

so long as they are not setting any of the values defined on the proxy initially, since those are read-only references.

The assertion added here is causing an addon test to fail, when using Ember canary where this change here just landed.

I don't really understand the reasoning here, at least from a user's perspective. Why shouldn't I be able to change a property of the hash even when it existed before?

In my mental model using {{hash foo="bar"}} should be the same as defining { foo: 'bar' }} on the class and passing this in the template. The latter allows mutating the hash in every possible way.

Even if there is some good reason to do this, this behavior must be documented, and changing it feels very much like a breaking change, which the failing test basically confirms. Not?

[pzuraq]

@simonihmig it would be very, very difficult to make this behavior work the way you described. It's really non-trivial, and introduces a lot of issues with timing and syncing the local state of the hash with your changes, to the state of the incoming references that defined the hash in the first place.

We wanted to avoid these extra complexities, which would likely introduce a host of their own small bugs and inconsistencies in our opinion. The hope was that users were treating hash as derived state, the way it was meant to be used, and were not using it as a store mutable state at all. If this is a common use case, then we may need to update the behavior to help that, but we really would rather avoid it.

If we do make the change, it is likely we will immediately deprecate the ability to do this, since it'll be much easier and simpler to maintain a hash which does not have this behavior.

[simonihmig]

I see, thanks for explaining.

So practically I myself am not really affected by this, as we don't have used patterns like this.

But this case was added to our test suite because of a real use case by @jelhan IIRC, right?

[pzuraq]

FWIW, if this is failing tests already in canary then I think we should at least re-evaluate our original decision. I'm going to discuss it with folks next week, some are currently out, we'll see how that goes before we do any backports or anything like that.

[jelhan]

My mental modal about {{hash}} helper was that it returned an object with the named arguments as properties. Basically I assumed that it could be implemented in user-land like this:

// app/helpers/hash.js

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

function hash(_, namedArgs) {
  return { ...namedArgs };
}

export default helper(hash);

I have used it whenever it improved readability to construct an object in the template rather than in the JavaScript file. Basically I considered that either constructing the object in a backing JavaScript class or using {{hash}} template helper has the same result.

// app/components/my-component.js

import Component from '@glimmer/component';

export default class MyComponent extends Component {
  get formModel() {
    return {};
  }
}

<PostForm @model={{this.formModel}} />

vs

<PostForm @model={{hash}} />

I often preferred the {{hash}} helper because it makes it often easier to reason about the code.

My mental modal about {{hash}} helper and derived state was similar to @localCopy decorator provided by tracked-toolbox. The local copy could be modified but would be thrown away if one of the arguments change.

Speaking in the past here because this issue and the discussion changed my mental model. I wasn't aware that {{hash}} helper evaluates arguments lazy. I wasn't aware that the returned object should not be mutated. Why is it not frozen in that case? I'm confused. My mental model around {{hash}} helper failed apart. Not sure if I'm the only one, who feels like this.

The impact on the apps I'm working on may not be that big though. Often the {{hash}} is wrapped inside an Ember Changeset if used as a model for a form. That should mitigate the affect as the object returned by {{hash}} helper is not modified. But I'm sure there are some cases, which would be broken.

It feels like a breaking change for me. Especially as the API documentation does not mention that mutating the returned object is discouraged. To be honest it sounds for more like a chance, which deserves a RFC.

[lifeart]

With set on hash it should throw error:

https://ember-twiddle.com/7ede4a6b8d3ce50698acc9d7fe57ebaa?openFiles=helpers.h%5C.js%2C

(replace

	return this.object = this.object || EmberObject.extend({...hash}).create();

with

	return this.object = this.object || hash

[pzuraq]

@jelhan to be fair, we were also not aware that hash worked this way when we made the original refactors/changes to references 😅 this was the issue with the previous reference system, every reference type could customize its own behavior, so you had weird edge cases where one specific type of reference would do something completely differently than every other type. So that was just a behavior we didn't know existed.

I think it's likely we'll have to keep the old behavior since things are failing, but in general I think you should reconsider the strategy with using hash as a mutable store of value. At the least, the following are all true:

1. hash always returned a normal object, not a TrackedObject or anything like that.
2. So, the only way to update the state on hash and make those updates reactive is by using Ember.get to read values on it, and using Ember.set to write values to it.
3. In the (distant) future, Ember.get and Ember.set will likely be deprecated. At the least, they are not considered the main way to access/update state in Octane and beyond.

Given that, I think this pattern will have to be re-evaluated either way. If this pattern were to continue in the future, I think we'd have to have hash return a TrackedObject (and introduce TrackedObject to Ember directly).