emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

[1.13+ Regression] Attempted to register a view with an id already in use #11925

Closed OrKoN closed 9 years ago

OrKoN commented 9 years ago

See the following jsbin. Click "Up" several times and you will get an error: Uncaught Error: Assertion Failed: Attempted to register a view with an id already in use: labelId.

The component having id=labelId is rendered twice in the template but there is a condition that ensures that there should be one element with this id at any moment:

{{#if show}}
  {{#x-label id="labelId"}}
    {{x-checkbox id="checkboxId"}}
  {{/x-label}}
{{else}}
  {{#each model as |item index|}}
    {{x-li tagName="li" item=item}}
    {{#if (eq index 0)}}
      <li>{{#x-label id="labelId"}}
        {{x-checkbox id="checkboxId"}}
      {{/x-label}}</li>
    {{/if}}
  {{/each}}
{{/if}}

Here is a working jsbin for Ember 1.12.1.

P.S. the condition inside the #each loop seems to be important to reproduce the issue. P.S.S. Tried to test with Ember 2.0.0-beta.4 but could not make it work because Ember could not find the eq helper

mixonic commented 9 years ago

@OrKoN this message seems to be catching a bug, no? Non-unique ids introduce a variety of unexpected behaviors.

OrKoN commented 9 years ago

@mixonic Can't I remove an element with an id and create it again in a different place with the same id? The non-unique id should never be rendered twice because of the boolean show condition but Ember thinks it is. Plus if rewritten like this it works: http://emberjs.jsbin.com/tejezavomu/1/edit?html,js,output but that's not my use case. In my app I render the component either separately or after the first item of the list. Of course, I can find a workaround but looks like some kind of bug to me.

pixelhandler commented 9 years ago

@OrKoN Since v1.13.6 was released today I tested your jsbin example with 1.13.6 and I still see the same issue.

OrKoN commented 9 years ago

@pixelhandler Thanks! I have tried to test with Ember 2.0.0-beta.4 but could not make the eq helper work...

noslouch commented 9 years ago

Hi, I have a similar issue with a reduced test case here: http://emberjs.jsbin.com/ladenej/edit?js,output

There is a set of radio input components rendered by an each loop. Their IDs are determined by the index and values are computed from an inventory on the controller. If the value of the enumerable used in the loop changes, the components are re-rendered.

The issue comes in when values are shared between rendered outputs. My hunch is that Glimmer is optimizing re-renders by looking at what is coming out, and keeps items in the DOM if they overlap.

This is happening on 1.13.8.

noslouch commented 9 years ago

FWIW: there are no error messages for ember 2.0.0: http://emberjs.jsbin.com/begoso/edit?js,output

Still errors in v2.

runspired commented 9 years ago

@mixonic inputs needing known IDs is a problem for ember right now. I bump into lots of scenarios like this where the input is unique, has an explicit ID (needed for labels), but throws this error when an if block changes or a model transition occurs (usually but not always with liquid-fire involved).

It's never that there is actually 2 instanced of an ID present, it's that one instance is being torn down but isn't removed yet, and another is being activated.

Something I've thought about is trying to build template helpers for generating unique IDs for form inputs. Something like this.

<label for={{(useInputId "foo-input-"}}>Foo</label>
<input type="text" id={{getInputId "foo-input="}}>

Where a given ID can only be consumed once before it's assigned a new uid, but this just feels to heavy a solution for avoiding teardown during animation/if block switching cases.

devinus commented 9 years ago

I am also experiencing this issue.

If I do this.set('items', []) and then do the render in an Ember.run.later it will work fine because it forces Glimmer to re-render after the old elements are removed. This is obviously bad.

devinus commented 9 years ago

This should also definitely be labeled as Glimmer and Regression.

devinus commented 9 years ago

Additionally, this is present even if I use Ember.run.scheduleOnce('afterRender').

rwjblue commented 9 years ago

I suspect that the initially reported issue is related to one of the memory leak bugs fixed in 1.13.10 that was causing an {{each}} in an if/else to not be torn down properly when traversing from the truthy to falsey blocks.

rwjblue commented 9 years ago

Upstream fixing PR with description of the issue: https://github.com/emberjs/ember.js/pull/12272.

Confirmed that the reported issue JSBin is working without error under 1.13.10: http://rwjblue.jsbin.com/noyuvib/edit?html,js

rwjblue commented 9 years ago

Looks like there is another scenario reported by @noslouch that I need to dig into also.

Reopening for now.

devinus commented 9 years ago

@rwjblue this is definitely happening on 1.13.10 for me.

devinus commented 9 years ago

@rwjblue For more information, it seems like I'm passing an index to a component within an each like @noslouch's example as well.

rwjblue commented 9 years ago

In the JSBin an array of numbers is iterated over via {{each}} and a component is created for each using the iteration index as the components elementId.

When you click a button, the underlying array of numbers is swapped out with another array of numbers triggering a rerender. During the rerender an assertion is triggered because two items exist with the same elementId. The following is a walk through of what is happening:

  1. When initially rendered we create components for [12000, 24000, 144000, 216000, 288000, 360000]
    • A new entry is added for 12000 (elementId of 0 is used)
    • A new entry is added for 24000 (elementId of 1 is used)
    • A new entry is added for 144000 (elementId of 2 is used)
    • A new entry is added for 216000 (elementId of 3 is used)
    • A new entry is added for 288000 (elementId of 4 is used)
    • A new entry is added for 360000 (elementId of 5 is used)
  2. Click on "change frequency"
  3. The templating layer begins processing the new list of items [1000, 2000, 12000, 18000, 24000, 30000]
    • A new entry is added for 1000 (elementId of 0 is used)
    • A new entry is added for 2000 (elementId of 1 is used)
    • The previously rendered item for 12000 is reused because the item being iterated is the same as the prior iteration (it has already been rendered with elementId of 0).
    • A new entry is added for 18000 (elementId of 3 is used)
    • The previously rendered item for 24000 is reused because the item being iterated is the same as the prior iteration (it has already been rendered with elementId of 1).
    • A new entry is added for 30000 (elementId of 5 is used)

As you can see you have actually told Ember to create two elements with an ID of 0, this is what the assertion is correctly telling you.


After doing the digging and tracking down what is actually happening here, this isn't something that we can fix unless we completely throw away any rerender optimizations that were added.

rwjblue commented 9 years ago

@stefanpenner - I'd like to close this, but your call.

noslouch commented 9 years ago

Thank you for the clear explanation of the Ember internals on this.

What about the use case @runspired brings up his comment? These are form elements with labels taking advantage of HTML's native click-label-and-focus-input behavior on labels and inputs with matching for and id attributes?

In my situation I ended up resolving this by rendering each "frequency" and displaying based on another property:

{{#if isMoreFrequent}}
    {{#each moreFrequentLevels as |level index|}}
        {{x-label for=index}}
        {{x-input id=index}}
    {{/each}}
{{else}}
    {{#each lessFrequentLevels as |level index|}}
        {{x-label for=index}}
        {{x-input id=index}}
    {{/each}}
{{/if}}

This seems like it's asking for performance lags, but maybe glimmer can handle it.

rwjblue commented 9 years ago

You need to avoid using id as an index by itself because once an element is created we never update its elementId (and I believe there is a warning/assertion somewhere if you try to change it), so when an item is reused it is likely to cause an issue because the index is used similarly to my walk through above.

However, if you use something from the object being yielded you are roughly guaranteed to avoid this type of conflict, because if the underlying object is reused it will still need/have the same value for id.


The point of the technique you are asking about is just that the for attribute has the same value as id on the input. The technique works perfectly well if you use something based on the object that is being yielded (because that the the basis of reuse by the rendering engine).

For example, you could do something like (assuming the thing yielded is an object and not a primitive):

{{! in the template }}
{{#each items as |item|}}
  <label for={{identity-for item}}>Foo</label>
  <input type="text" id={{identity-for item}}>
{{/each}}
// app/helpers/identity-for.js
import Ember from 'ember';

export default Ember.Helper.helper(function([object]) {
  return Ember.guidFor(object);
});
stefanpenner commented 9 years ago

@rwjblue I agreed with you.

Although I suspect this may cause some upgrade pains. This feels like a big :footprints: :gun:, relying on dynamically toggling ID's settling correctly, is an unintended hazard. I apologies that we in the past exposed such a hazard.

devinus commented 9 years ago

@noslouch's example is exactly what I'm doing. label for and input id. What I'm wondering is why isn't the element just being reused? Why is it trying to create an element with the same ID?

devinus commented 9 years ago

Also, can this be solved by using e.g. key="@index"?

devinus commented 9 years ago

UPDATE: My hunch was correct. Using key='@index' solves this problem, I'm guessing because it no longer tries to tie the element to previous elements in the array? @rwjblue

devinus commented 9 years ago

Also, using key='@index' seems MUCH faster in this case.