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

Strange 1.13 Canary Deprecation #11156

Closed
daltones opened this Issue May 14, 2015 · 9 comments

Comments

Projects
None yet
9 participants
@daltones

I've been using this very useful pattern in 1.11, so as in 1.12-beta3 too:

<label for={{view.myField.elementId}}>My Field:</label>
{{input viewName="myField"}}

Now in 1.13.0-beta.1+canary.9936dc4e, with Glimmer, I'm having this deprecation message:

You modified view.myField.elementId twice in a single render. This was unreliable in Ember 1.x and will be removed in Ember 2.0

I am doing no modifications in elementId at all.

@daltones daltones changed the title from 1.13 canary deprecation to Strange 1.13 Canary Deprecation May 14, 2015

@daltones

This comment has been minimized.

Show comment
Hide comment

Sorry, I forgot to put on JS Bin. Here it is:

http://emberjs.jsbin.com/nujofuyigi/1/edit?html,js,console,output

@ShogunPanda

This comment has been minimized.

Show comment
Hide comment
@ShogunPanda

ShogunPanda May 17, 2015

Yup, it's happening to me as well.

Yup, it's happening to me as well.

@mixonic mixonic added this to the 1.13.0 milestone May 17, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue May 18, 2015

Member

The deprecation is actually correct. It has bitten me, and I have mistakenly suggested folks use viewName for this exact use case in the past.


First let me try to explain why the deprecation is correct:

  1. <label for={{view.myField.elementId}}>My Field:</label> executes, and the view.myField.elementId is undefined.
  2. {{input viewName="myField"}} executes
    1. a new Ember.TextField is created for the {{input}} invocation
    2. the view.myField property is set
  3. The view.myField property changing from undefined to the new Ember.TextField instance triggers a rerender.

Now, lets talk about a better strategy for this (and I am truly sorry if I gave the original idea to you!): http://emberjs.jsbin.com/rwjblue/511/edit

  1. Make a computed property that will determine the id for the {{input}}.
  2. Use that computed property for both the <label> and the {{input id=somePropertyName}}.

The benefits are multiple:

  • we use a much easier to follow pattern (the percentage of folks that know viewNames is relatively small) by controlling the initial value of the input's id
  • we avoid triggering nested rerenders
  • No more deprecation!

Hopefully, that resolves the issue for you (I'm happy to reopen if it does not)...

Member

rwjblue commented May 18, 2015

The deprecation is actually correct. It has bitten me, and I have mistakenly suggested folks use viewName for this exact use case in the past.


First let me try to explain why the deprecation is correct:

  1. <label for={{view.myField.elementId}}>My Field:</label> executes, and the view.myField.elementId is undefined.
  2. {{input viewName="myField"}} executes
    1. a new Ember.TextField is created for the {{input}} invocation
    2. the view.myField property is set
  3. The view.myField property changing from undefined to the new Ember.TextField instance triggers a rerender.

Now, lets talk about a better strategy for this (and I am truly sorry if I gave the original idea to you!): http://emberjs.jsbin.com/rwjblue/511/edit

  1. Make a computed property that will determine the id for the {{input}}.
  2. Use that computed property for both the <label> and the {{input id=somePropertyName}}.

The benefits are multiple:

  • we use a much easier to follow pattern (the percentage of folks that know viewNames is relatively small) by controlling the initial value of the input's id
  • we avoid triggering nested rerenders
  • No more deprecation!

Hopefully, that resolves the issue for you (I'm happy to reopen if it does not)...

@jesenko

This comment has been minimized.

Show comment
Hide comment
@jesenko

jesenko May 20, 2015

@rwjblue I tried modifying your example to generate elementId for the input component within parent template: http://emberjs.jsbin.com/layuga/1/edit?html,js,console

This would eliminate the need to explicitly define computed properties, removing some boilerplate. However id property does evaluate handlebars expression, rendering

<input id="input-{{elementId}}" type="text" class="ember-view ember-text-field">

Is there a fundamental reason why id setter does not behave as e.g for attribute?

jesenko commented May 20, 2015

@rwjblue I tried modifying your example to generate elementId for the input component within parent template: http://emberjs.jsbin.com/layuga/1/edit?html,js,console

This would eliminate the need to explicitly define computed properties, removing some boilerplate. However id property does evaluate handlebars expression, rendering

<input id="input-{{elementId}}" type="text" class="ember-view ember-text-field">

Is there a fundamental reason why id setter does not behave as e.g for attribute?

@jesenko

This comment has been minimized.

Show comment
Hide comment
@jesenko

jesenko May 20, 2015

@rwjblue Got it working with custom concat helper...
http://jsbin.com/layuga/2/

jesenko commented May 20, 2015

@rwjblue Got it working with custom concat helper...
http://jsbin.com/layuga/2/

@dwickern

This comment has been minimized.

Show comment
Hide comment
@dwickern

dwickern May 27, 2015

Thanks @jesenko, your solution is much more palatable.

When components eventually use angle brackets we can get rid of the helper
{{my-component id=(concat-me "input-" elementId) value=attrs.value}}
<my-component id="input-{{elementId}}" value={{attrs.value}}>

(how will angle brackets work for the {{input}} helper?)

Thanks @jesenko, your solution is much more palatable.

When components eventually use angle brackets we can get rid of the helper
{{my-component id=(concat-me "input-" elementId) value=attrs.value}}
<my-component id="input-{{elementId}}" value={{attrs.value}}>

(how will angle brackets work for the {{input}} helper?)

@SaladFork

This comment has been minimized.

Show comment
Hide comment
@SaladFork

SaladFork Jul 13, 2015

Contributor

Just discussed this with someone via the Ember Slack. In most cases you can replace connecting a <label with a for= attribute to a form element with a name= or id= by simply wrapping both the label content and the form element within <label>.

To use your example, you'd instead do:

<label>
  My Field: {{input ...}}
</label>

This subverts the need for a unique name, while maintaining all the niceties of labels such as activating the form element when the label is clicked.

Contributor

SaladFork commented Jul 13, 2015

Just discussed this with someone via the Ember Slack. In most cases you can replace connecting a <label with a for= attribute to a form element with a name= or id= by simply wrapping both the label content and the form element within <label>.

To use your example, you'd instead do:

<label>
  My Field: {{input ...}}
</label>

This subverts the need for a unique name, while maintaining all the niceties of labels such as activating the form element when the label is clicked.

@jamesarosen

This comment has been minimized.

Show comment
Hide comment
@jamesarosen

jamesarosen Aug 27, 2015

Contributor

https://github.com/Netflix/ember-nf-graph suffers the same problem, but doesn't use viewName or custom IDs.

Some possibly relevant code:

// graph-has-graph-parent.js:
init() {
  this._super(...arguments);
  var graph = this.nearestWithProperty('isGraph');
  this.set('graph', graph);
}

// nf-graph.js
init() {
  this._super(...arguments);
  var graph = this.get('graph');
  if(graph) {
    graph.registerGraphic(this);
  }
},

// graph-registered-graphic.js
init() {
  this._super(...arguments);
  var graph = this.get('graph');
  if(graph) {
    graph.registerGraphic(this);
  }
},
Contributor

jamesarosen commented Aug 27, 2015

https://github.com/Netflix/ember-nf-graph suffers the same problem, but doesn't use viewName or custom IDs.

Some possibly relevant code:

// graph-has-graph-parent.js:
init() {
  this._super(...arguments);
  var graph = this.nearestWithProperty('isGraph');
  this.set('graph', graph);
}

// nf-graph.js
init() {
  this._super(...arguments);
  var graph = this.get('graph');
  if(graph) {
    graph.registerGraphic(this);
  }
},

// graph-registered-graphic.js
init() {
  this._super(...arguments);
  var graph = this.get('graph');
  if(graph) {
    graph.registerGraphic(this);
  }
},
@blessenm

This comment has been minimized.

Show comment
Hide comment
@blessenm

blessenm Aug 28, 2015

Contributor

Im also getting into trouble with this while trying to set aria values to input fields. I guess setting a default value and then it getting changes when the model values flow down cause this issue in my case.

Doesn't the runloop ensure that the final value is only used for rendering?

Contributor

blessenm commented Aug 28, 2015

Im also getting into trouble with this while trying to set aria values to input fields. I guess setting a default value and then it getting changes when the model values flow down cause this issue in my case.

Doesn't the runloop ensure that the final value is only used for rendering?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment