V3.28.12 fix polymorphism

[Skwai]

Description

Notes for the release

[runspired]

👋🏻 hi, I fixed the branch base for you to target lts-3-28. However, I don't think we want to disable this assert, can you explain why you are looking to do this?

[Skwai]

However, I don't think we want to disable this assert, can you explain why you are looking to do this?

@runspired Thanks. We had to do this as Ember Data polymorphism enforces inheritance. I/we feel like this is a mistake. Polymorphism shouldn't rely on inheritance, it should rely on the implementation of a common interface.

This becomes a problem when we have Ember Data models that already inherit from other parent classes. We tried using the Ember Mixin approach but that doesn't seem to work either:

ember-decorators/ember-decorators#422

Our code works when building for production but breaks when DEBUG is true. FWIW I think this check should emit a warning, not break the code. The code previously worked when we used an earlier version of ember-data so in order to fix things ASAP we had to patch ED.

This RFC seems to relate to the issue:

Allow setting polymorphic relationships without inheritance and mixins.

https://rfcs.emberjs.com/id/0793-polymporphic-relations-without-inheritance/

[runspired]

To use EmberData polymorphism in that way without mixins you definitely need to be using 4.7+ (where that RFC was implemented). The traits/structure based approach will sometimes seem to work but silently fails in pretty significant ways that could negatively impact your application state and correctness and lead to data loss if a record were to be saved.

This error is telling you something real: that your application is misconfigured. The Mixin approach does work, just not with native class syntax which creates an inheritance chain that EmberData can't appropriately walk.

You'll want to use this hybrid syntax:

import Model, { attr }  from '@ember-data/model';
import Commentable from 'my-app/mixins/commentable';

export default class Post extends Model.extend(Commentable) {
  @attr title;
}

Or maintain models in classic syntax for the time being. Generally speaking classic syntax for models is around 30x better performance anyway 😬, and once you get to EmberData 4.7+ you'll be able to refactor to not use mixins / once you get to EmberData 4.12+ you'll be able to use something with much nicer syntax (we're planning on making the model replacement we ship in 5.x compatible with EmberData 4.12 to ease upgrade cycles on the path to Polaris).

[Skwai]

We're planning to move to 4.x, but just moved to the 3.x LTS to address deprecations before moving to 4.x so we're a way off moving to 4.12.

Generally speaking classic syntax for models is around 30x better performance anyway

That's concerning! Is that documented anywhere? We have a large application with dozens of models so refactoring to use classic syntax isn't really viable at this stage.

[runspired]

That's concerning! Is that documented anywhere? We have a large application with dozens of models so refactoring to use classic syntax isn't really viable at this stage.

It's a bit of a well-understood side-effect of decorators, especially ones that have to interop between both classic and native class syntaxes (like attr/hasMany/belongsTo needs to). The fix is to (1) migrate the community to using stage-3 decorators and (2) enable a migration to decorators that don't support classic at all. I'm probably going to do (2) as part of the migration path for folks as it is, maybe I could get that into 4.12 / backport to 4.4 and 4.8 🤔

This side-effect affects models far more than components mostly due to usage volume. Both concepts take a hit, but where components can refactor to the simpler glimmer base class and thus gain-some/lose-some with models its only lose at the moment and models tend to be instantiated far more frequently and in much larger numbers than components do. So if your app is perf-sensitive, leaving models fully classic until there are other options available to you is a good option. If it can absorb a bit of a hit in this area in favor of the nice DX of native class, then probably that's the better option as over time we will be able to holistically fix the perf (and if it matters to you a ton then please take on this RFC emberjs/rfcs#876)