emberjs / ember.js

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

[Bug] can not dynamically assign prefix attribute in templates #19369

Open nvdk opened 3 years ago

nvdk commented 3 years ago

🐞 Describe the Bug

In our application we're working with rdfa annotations. When we try to assign the prefix attribute on an element, this raises a runtime error in emberjs. This happens both in glimmer components as in regular routes with a controller and template.

🔬 Minimal Reproduction

The following is a very basic example using a controller and a template https://gist.github.com/nvdk/9bd783bf8e5a55170ceb4a61887d3d67 https://ember-twiddle.com/9bd783bf8e5a55170ceb4a61887d3d67

😕 Actual Behavior

Runtime error:

Uncaught TypeError: setting getter-only property "prefix"
    Ember 29
        __setProperty
        set
        setDynamicAttribute
        <anonymous>
        evaluate
        evaluateSyscall
        evaluateInner
        ...

🤔 Expected Behavior

The attribute is assigned just like any other attribute.

🌍 Environment

Tested on

➕ Additional Context

We can work around the issue with the did-insert modifier and then setting the attribute using element.setAttribute, but would prefer just being able to assign it in the template.

Windvis commented 3 years ago

I did some sleuthing and I think I figured out the issue.

The Element class has a prefix property which is read-only, but since we're trying to set an attribute that shouldn't be an issue (I think).

The problem is that the Glimmer VM has the following logic. It checks if the slotName exists as a property on the element, and if so, returns prop as the type. Since prefix does exist as a prop, this method returns prop even though the attribute isn't related to the property at all.

This type is then used to determine the following actions and somewhere along that code path the property is actually set and the getter error is triggered.

A potential way to fix this is to catch this edge case in the preferAttr util that is being used. If that util returns true the type will be forced to attr and the problem will not occur.

There might be better ways to solve it though. I'm unfamiliar with the codebase (and browser specifications in general for that matter :grimacing:).

rwjblue commented 3 years ago

Depending on usage in fastboot or not, you could also consider writing a custom modifier that forces it to be an attribute.

<div {{attr prefix=this.generatedPrefix}} >

Where attr is basically something like:

// /app/modifiers/attr.js
import { modifier } from 'ember-modifier';

export default modifier((element, positional, named) => {
  for (let key in named) {
    element.setAttribute(key, named[key]);
  }
});

^ is possible to write and work today without changes in Ember / GlimmerVM, this is basically the old {{bind-attr}} concept.

nvdk commented 2 years ago

just a short update: this is still an issue on ember 3.28, but we're using a modifier as workaround as suggested

Windvis commented 2 years ago

I still think that prefix is one of those 'spec outliers' for which preferAttr should return true. form is also handled by preferAttr and it's the exact same situation where it can't be set as a prop since that prop is read-only. The difference here is that it should be set as an attribute for all elements instead of only elements related to forms.

Having built-in support for this also means that the attribute is set properly in FastBoot which the modifier version simply can't do.