Skip to content
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

3.11 regression when using "elementId" in templates #18147

Closed
kanongil opened this issue Jun 26, 2019 · 11 comments · Fixed by #18807
Closed

3.11 regression when using "elementId" in templates #18147

kanongil opened this issue Jun 26, 2019 · 11 comments · Fixed by #18807

Comments

@kanongil
Copy link
Contributor

After upgrading my code to 3.11, I receive an error: "Error: Changing a view's elementId after creation is not allowed".

Since I don't change the elementId anywhere in my code, and the new value is identical to the recorded value, I have explored further and found it to be a new bug.

I can trigger the issue with the following:

templates/application.hbs

{{my-test elementId="my-id" handler=(action test) value=myValue}}

controllers/application.js

export default Controller.extend({
    myValue: 123,

    test() {
        console.log('not used...');
    },

    init() {
        this._super(...arguments);

        next(() => {
            this.set('myValue', 321);
        });
    }
});

templates/components/my-test.hbs

Empty...

You can find failing copy here: https://github.com/kanongil/ember-fail

Note that I also tested it with angle-bracket components, and here it doesn't fail until I make the value {{mut}}.

It seems that the bug is triggered by a combination of using elementId, an action handler, and a mutable value that is set after it has been rendered.

@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2019

Thank you for reporting!

@rwjblue rwjblue added the Bug label Jun 26, 2019
@boris-petrov
Copy link
Contributor

@rwjblue - is this on the roadmap for a fix soon? We cannot update past 3.10 because of this bug and I'll have to start thinking for workarounds if this is not something that is going to be fixed in a patch release. Thanks!

Copy link
Member

rwjblue commented Jul 3, 2019

I'm definitely keen on getting a fix into a patch release, but I haven't had a chance to dig in to find the right fix yet.

@boris-petrov
Copy link
Contributor

@rwjblue - ok, thanks, no worries, take your time, I just wanted to know if I had to think of workarounds.

@simonihmig
Copy link
Contributor

I remember being able to "fix" this by passing id instead of elementId in a template (https://github.com/kaliber5/ember-bootstrap/pull/807/files), when tests started to fail in canary (at that time).

I contributed this to passing elementId as the "wrong" way to do it, i.e. pass id in a template as this makes it look more natural, HTML-like - and use elementId within the component's JS (if needed). But it seems passing elementId also is valid (see https://api.emberjs.com/ember/3.11/classes/Component/properties/elementId?anchor=elementId), so we have two equivalent APIs (in template-land) for the same thing (which is a bit confusing).

@boris-petrov
Copy link
Contributor

@simonihmig - thanks for the idea! I was finally able to update to 3.11. :)

I still do think that this is a bug though and has to be researched.

Copy link
Member

rwjblue commented Jul 30, 2019

Definitely agree that this needs to be fixed, just haven't had time to dig into it.

@barryofguilder
Copy link

I just ran into this when trying to upgrade my app yesterday 😀 Looks like the workaround will work for us too, so thanks @simonihmig!

CvX added a commit to discourse/discourse that referenced this issue Jan 27, 2020
@Samsinite
Copy link

Samsinite commented Jan 30, 2020

Just ran into this yesterday as well when upgrading to 3.12, glad to at least see a work-around :). Is id going to continue to be supported? Didn't know that was even a property we could override and it isn't documented :(.

CvX added a commit to discourse/discourse that referenced this issue Feb 5, 2020
* DEV: Use Ember 3.12.2
* Add Ember version to ThemeField's DEPENDENT_CONSTANTS
* DEV: Use `id` instead of `elementId` (See: emberjs/ember.js#18147)
* FIX: Don't leak event listeners (bug introduced in 999e2ff)
@chriskrycho
Copy link
Contributor

@rwjblue and I just figured out what we need to do to fix, and I'll have a PR with both failing tests and a fix for it in a little bit! Thanks for the reports and the workarounds!

@rwjblue
Copy link
Member

rwjblue commented Mar 10, 2020

Thanks for working on this @chriskrycho! Assuming the fix is as "simple" as we expect, I think we'll be able to backport to 3.16 and 3.12.

kategengler pushed a commit that referenced this issue Mar 16, 2020
When refactoring from observers to setters during the 3.11 release
(in 405d423), an apparently-transparent change caused a regression
around invocations of components including `elementId=...`. *Any*
rerender of the component which included `elementId` assignment now
triggered the assertion, rather than *only* changes which actually
updated the value of `elementId`, because all invocations strigger
a `set` on the property.

The simplest reproduction of this bug (given a component `foo-bar`):

    {{foo-bar
      elementId='stable'
      changingValue=this.fromBackingClass
    }}

Changing the value `fromBackingClass` on the backing class *always*
triggers the assertion, even though it is actually impossible for
it to change the `elementId` value, because the assertion in the
setter throws regardless of what the value is. (The observer-based
did not throw in the same conditions because it would not retrigger
when the observed key on the view did not change.)

The fix is to check equality between the passed `elementId` value
and the previously set value, and only throw if they differ.

Fixes #18147

(cherry picked from commit da8c466)
oblakeerickson added a commit to discourse/discourse that referenced this issue Mar 18, 2020
I think this issue is caused by a current regression in ember

emberjs/ember.js#18147

but using `id` works just fine in templates. This also appears to be the
only template file we are using `elementId` directly in the template.
rwjblue pushed a commit that referenced this issue Mar 23, 2020
When refactoring from observers to setters during the 3.11 release
(in 405d423), an apparently-transparent change caused a regression
around invocations of components including `elementId=...`. *Any*
rerender of the component which included `elementId` assignment now
triggered the assertion, rather than *only* changes which actually
updated the value of `elementId`, because all invocations strigger
a `set` on the property.

The simplest reproduction of this bug (given a component `foo-bar`):

    {{foo-bar
      elementId='stable'
      changingValue=this.fromBackingClass
    }}

Changing the value `fromBackingClass` on the backing class *always*
triggers the assertion, even though it is actually impossible for
it to change the `elementId` value, because the assertion in the
setter throws regardless of what the value is. (The observer-based
did not throw in the same conditions because it would not retrigger
when the observed key on the view did not change.)

The fix is to check equality between the passed `elementId` value
and the previously set value, and only throw if they differ.

Fixes #18147

(cherry picked from commit da8c466)
richgt added a commit to richgt/ember.js that referenced this issue Jun 25, 2020
richgt added a commit to richgt/ember.js that referenced this issue Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants