-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Glimmer2] Implement get helper #13173
Conversation
} | ||
|
||
get(propertyKey) { | ||
throw new Error("get() is not implemented"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't find when get()
is supposed to be called. It's not invoked with tests.
I believe I'll find the answer but any guidance appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe get is used for something that is opaque from the consumer.
{{!-- application.hbs --}}
{{foo-bar as |value|}}
{{value.foo}}
{{/foo-bar}}
{{!-- templates/components/foo-bar.hbs --}}
{{yield (get someObj somePathThatPointsToAnotherObj)}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to be a test for this. I would suggest adding a test that does this type of yielding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, a Reference
is an abstract data type with the following interface:
interface Reference<T> {
value(): T;
}
A reference represents a "computation" (similar to a "stream"), and calling .value()
will return the current value of that computation. For example, a reference representing {{foo}}
will return the current value of the foo
property in the template's context. This gives you a stable value/wrapper (hence the name "reference") to hold on to that you can query in the future to re-render the template with the latest value as needed.
A PathReference
is an extension to the system:
interface PathReference<T> extends Reference<T> {
get(key: string): PathReference<any>;
}
Implementing the PathReference
interface means that the value()
s of the computation supports a conceptual get
operation. For the purpose of understanding this, you can apply the Ember.get
semantics here – i.e. the value()
s returned by this computation makes sense as input to the first argument of Ember.get
.
Alternatively, you can also think about the get
semantics in terms of the templates. Anything foo
that can be used in a {{foo.bar}}
position must implement PathReference
. In this case, {{foo.bar}}
will take the {{foo}}
reference as input and call .get("bar")
on it to produce another stable reference (another PathReference
, actually) representing {{foo.bar}}
.
In practice, almost anything that can be used in a template needs to implement PathReference
, because in handlebars almost anything can be used in that position. In addition to @chadhietala's example, there are a few other ways that a get
helper can get into that position: {{(get foo bar).baz}}
, {{#with (get foo bar) as |gotBar|}} {{gotBar.baz}} {{/with}}
, etc.
In this case, the reference you are implementing is representing the result of the (get foo bar)
computation over time, where foo and bar themselves are also PathReferences
. The get(path)
method (confusingly named in this context) should produce a reference representing the (get foo bar).path
computation.
Putting everything together, I think something like this would work:
...
toReference(args) {
let sourceReference = args.positional.at(0); // foo in (get foo bar)
let propertyPathReference = args.positional.at(1); // bar in (get foo bar)
if (isConst(propertyPathReference)) {
// (get foo "bar"), so `propertyPathReference` is a reference whose value cannot change ever
// this is pretty pointless, you could have just typed `{{foo.bar}}`
return sourceReference.get(propertyPathReference.value());
} else {
return GetHelperReference(sourceReference, propertyPathReference);
}
}
...
class GetHelperReference {
// ...mostly following your implementation
get(path) {
new PropertyReference(this, path);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to understand is why you need PathReference
in the first place. Certainly, you can just call .value()
on the parent and use a regular property lookup on the reified value.
The reason for PathReference
is that the underlying root reference might have additional information about when you need to reify the value for paths derived off of it:
- in Ember, if nobody called
.set()
on the parent object, you don't need to recompute the value. - in Ember, if you're looking at a computed property, you can rely on the computed property cache.
- when working with immutable data structures, you only need to recompute the value if the root of the data structure has been replaced.
- when working with primitive values, the
get()
virtually always produces a null value. - when working with non-configurable, non-writable values, you only need to compute the value once.
In other words, there's all kinds of information in the JavaScript data layer about how to most-efficiently derive a path off of a value, and PathReference
allows the owner of the data to encode that strategy.
In Ember, this results in significant efficiency gains when Ember objects are used. Those gains would be portable to other data management strategies like immutable.js without forcing every data structure in your entire app to use Ember objects or immutable.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you guys for such comprehensive explanations. It's definitely more than I could expect!
02c1d4b
to
57a387b
Compare
Sorry for a long progress. Some irrelevant tasks take much time. I've added Currently when you try to
when the
That's why null check tests are marked as |
this.assertText('[yellow-redish] [yellow-redish]'); | ||
} | ||
|
||
['@test should be able to get an object value with a GetStream key']() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions for a better name here? GetStream
looks like an htmlbars
term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I agree. I think the GetStream
is just referring to the nested get helper. Maybe just "should be able to use the result of a get
helper as the key"?
@ro0gr I probably need to circle back to this after EmberConf (at least after my talk 😄). Meanwhile, I wrote up the stuff we talked about in more details here: glimmerjs/glimmer-vm@master...chancancode:docs |
@chancancode no problem. I'm not in hurry. |
import TextField from 'ember-views/views/text_field'; | ||
import Component from 'ember-views/components/component'; | ||
|
||
moduleFor('Helpers test: {{get}}', class extends RenderingTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we usually have a new blank line here for the new tests
colors: null, | ||
key: null | ||
}); | ||
this.assertText('[] []'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably do the I-N-U-R steps here to test that it can go from null to something else (and back)
@ro0gr sorry for the delay! I left some comments. Looks good to me otherwise! |
Ah, one more thing – can you rebase and squash? |
84686eb
to
ddb382d
Compare
Thank you @wycats for merging this work and sorry again for such a long progress on finalizing this. Anyway I've walked through your comments @chancancode. There were so much small issues. Hope I've fixed all of them in the latest commit. |
ddb382d
to
bd03f06
Compare
Sorry we dropped the ball linking that this PR was superseded by #13264 which addressed the above comments and was merged yesterday. |
@krisselden I've seen this @wycats pr.
|
Should it be delivered via a new pr or left as it is? |
@ro0gr thank you |
@krisselden and thank you 😄 |
Helpers are required to return `PathReference`s`, because their return values can be later used in lookup positions, e.g. ``` {{#with (my-helper) as |foo|}} {{foo.bar}} <--- referenceFromMyHelper.get('bar') {{/with}} ``` See emberjs/ember.js#13173 (comment) for more details. We were previously using `Function.prototype.call` to invoke helpers, which is typed to be `call(thisArg: any, args: any[]): any` which caused us to not get a TypeError from the change (because the return value is `any`). This commit fixed that as well.
Helpers are required to return `PathReference`s`, because their return values can be later used in lookup positions, e.g. ``` {{#with (my-helper) as |foo|}} {{foo.bar}} <--- referenceFromMyHelper.get('bar') {{/with}} ``` See emberjs/ember.js#13173 (comment) for more details. We were previously using `Function.prototype.call` to invoke helpers, which is typed to be `call(thisArg: any, args: any[]): any` which caused us to not get a TypeError from the change (because the return value is `any`). This commit fixed that as well.
Left to do:
{{input}}
helperinput
andmut
get()
for theGetHelperReference
get()
null
forsourceReference
andpropertyPathReference
(marked tests as[htmlbars]
only)