[Glimmer2] Add mut helper and update readonly helper #13541
Conversation
Joelkang
changed the title
| @@ -76,7 +76,7 @@ moduleFor('@htmlbars Mutable bindings integration tests', class extends Renderin | |||
| } | |||
|
|
|||
| // See https://github.com/emberjs/ember.js/commit/807a0cd for an explanation of this test | |||
| ['@test using a string value through middle tier does not trigger assertion'](assert) { | |||
| ['@htmlbars using a string value through middle tier does not trigger assertion'](assert) { | |||
There was a problem hiding this comment.
Is this because the mut wrapping isn't happening?
There was a problem hiding this comment.
you're right; updated
|
☔ The latest upstream changes (presumably 3a61d98) made this pull request unmergeable. Please resolve the merge conflicts. |
Joelkang
force-pushed
the
mut
branch
2 times, most recently
from
1d15660
to
863bf3a
Compare
| this.assert.equal(get(this.context, 'val'), 12, 'upstream attribute is not updated'); | ||
| } | ||
|
|
||
| ['@htmlbars {{mut}} of a {{readonly}} mutates only the middle and bottom tiers']() { |
There was a problem hiding this comment.
@krisselden @chancancode so this is still failing in glimmer.
To make it backwards compatible for curly components, what ember-htmlbars seems to be doing is disconnecting the readonly if it is updated from a child, such that get(child, 'parentAttr') is no longer the same as child.attr.parentAttr. But when a change comes from the upstream property, the value correctly updates.
I've tried a variety of ways of disconnecting the value from the reference and reconnecting it, but nothing I've tried really works .It also seems like there could be multiple places this logic can go into (should it be in process-args or mut?) Any direction here would be helpful.
Joelkang
force-pushed
the
mut
branch
2 times, most recently
from
faf561e
to
182adfd
Compare
| } | ||
|
|
||
| update(val) { | ||
| this._ref[UPDATE](val); |
There was a problem hiding this comment.
Doesn't the value need to be reflected on both sides?
There was a problem hiding this comment.
So I added tests to verify the value of component.attrs.myAttr.value and component.get('myAttr')and they all seem to be passing after the update() without any code changes. Is that what you meant?
Joelkang
force-pushed
the
mut
branch
2 times, most recently
from
c62db4f
to
9e16add
Compare
Joelkang
changed the title
| @@ -2,7 +2,8 @@ import run from 'ember-metal/run_loop'; | |||
| import { computed } from 'ember-metal/computed'; | |||
| import isEnabled from 'ember-metal/features'; | |||
| import { subscribe, unsubscribe } from 'ember-metal/instrumentation'; | |||
| import { INVOKE } from 'ember-htmlbars/keywords/closure-action'; | |||
| import { INVOKE as HTMLBARS_INVOKE } from 'ember-htmlbars/keywords/closure-action'; | |||
| import { INVOKE as GLIMMER_INVOKE } from 'ember-glimmer/utils/references'; | |||
There was a problem hiding this comment.
Would be nice to put this forking into one of the helper files so that the test can just import { INVOKE } from '../../utils/helpers'; and it would get either the HTMLBars or Glimmer symbol.
|
☔ The latest upstream changes (presumably 0f2d9e6) made this pull request unmergeable. Please resolve the merge conflicts. |
| let name = this._debugContainerKey.split(':')[1]; | ||
| let value = get(this, key); | ||
| throw new Error(strip` | ||
| let name = this._debugContainerKey.split(':')[1]; |
There was a problem hiding this comment.
This is only used for the assertion, but as written it will not get stripped. Can you refactor to avoid splitting in prod builds where the assertion is not needed? Something like:
let value = get(this, key);
runInDebug(function() {
let name = this._debugContainerKey.split(':')[1];
assert(strip`
Cannot set the \`${key}\` property (on component ${name}) to
\`${value}\`. The \`${key}\` property came from an immutable
binding in the template, such as {{${name} ${key}="string"}}
or {{${name} ${key}=(if theTruth "truth" "false")}}.
`, reference[UPDATE]);
});
reference[UPDATE](value);
|
This is looking really good. I am no expert in the ember-glimmer techniques (I'll leave that to the rest of the team) but I am curious what you see as the roadmap for this PR. What is still outstanding, what is blocking, etc... One thing I was noticing/thinking about was the auto-mut AST transform, should that be reenabled in this PR? |
|
Rebased and addressed your concerns, @rwjblue . As far as a I can tell there's nothing really blocking this PR, once these issues are resolved
|
| } else if (actionType !== 'function') { | ||
| throw new EmberError(`An action could not be made for \`${rawAction}\` in ${target}. Please confirm that you are using either a quoted action name (i.e. \`(action '${rawAction}')\`) or a function available in ${target}.`); | ||
| // FIXME: Is there a better way of doing this? |
There was a problem hiding this comment.
Can you change this to a TODO:? I discussed with @chancancode and this seems okish for now, and we can circle back to it later...
|
I was chatting with @chancancode about this abit today, he is going to try to give it more review sometime next week... |
|
☔ The latest upstream changes (presumably #13492) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #13680) made this pull request unmergeable. Please resolve the merge conflicts. |
Implemented based on comments in #13502
muthelper{{action attrs.somethingUndefined}}returns PropertyReference as opposed to UNDEFINED_REFERENCE, causing a call-time error rather than a create time error