record-data implementation for ember-data 2.8+

[dwickern]

An attempt at E-D 2.8 compatibility using record-data and public APIs.

Fragments are stored internally as RecordData:

type Fragment = RecordData | null;
type FragmentArray = RecordData[] | null;
type Array = unknown[] | null;

type FragmentData = Record<string, Fragment | FragmentArray | Array>;

interface FragmentRecordData {
  // fragment dirty state, analogous to _attributes
  _fragments: FragmentData; 

  // fragment in-flight state, analogous to _inFlightAttributes
  _inFlightFragments: FragmentData;

  // fragment canonical state, analogous to _data
  _fragmentData: FragmentData;
}

FragmentRecordData no longer references Fragment records. Fragment records are created on demand, either by calling get on the fragment attribute or objectAt on a fragment array.

FragmentArray and StatefulArray are based on ember-data's ManyArray.

/cc @runspired #406

[dwickern]

I couldn't figure out how to make snapshots work. Snapshots call get on the record, which returns the Fragment instance. https://github.com/emberjs/data/blob/b16011382b2321171b18ab4d23eeea26ad1d7a7c/packages/store/addon/-private/system/snapshot.ts#L165

How can we make snapshots use the serialized fragment value instead?

[knownasilya]

cc @runspired regarding that last comment, not sure if you have any ideas.

[knownasilya]

@VincentMolinie I know you have contributed quite a bit, do you think this could be rebased against your changes? Or would it be better to open a new PR and pull in these changes onto master?

[VincentMolinie]

@knownasilya yes of course, do you want me to do a new PR with theses changes ?

[knownasilya]

@VincentMolinie that's up to you, what you think is easiest.

[dwickern]

I'll see about merging master into this PR. It doesn't look too bad.

[dwickern]

It will be tricky to support typeKey as a function for this use-case:

https://github.com/adopted-ember-addons/ember-data-model-fragments/blob/bd9042d71f3ada648b1bc3a838db3e5b5847e80e/tests/dummy/app/models/component.js#L6-L9 https://github.com/adopted-ember-addons/ember-data-model-fragments/blob/bd9042d71f3ada648b1bc3a838db3e5b5847e80e/tests/unit/serialize_test.js#L31-L43

The ComponentOptions typeKey depends on its owner's type attribute. Do we initialize the owner's attributes and then its fragments so that we can pass a partially-constructed owner to typeKey? What if typeKey depends on the contents of some other fragment?

[richgt]

@dwickern - I'm running into issues on 3.28 now, and probably need this version of Fragments. What's the status of this PR? Is there anything I can do to help?

[esbanarango]

@dwickern - I'm running into issues on 3.28 now, and probably need this version of Fragments. What's the status of this PR? Is there anything I can do to help?

Same here! It would be great to have at least a beta version out of this PR 🙏

[dwickern]

I got the tests passing against E-D 3.28. My merge missed a change from #432. However typeKey-as-a-function is not implemented by this PR and I'm not sure how it can be supported. We need to know the typeKey in order to construct the fragment, but we need to pass the fragment to the typeKey function??

Tests fail against older E-D versions because they need a feature flag enabled. I'd rather drop support for older versions.

[knownasilya]

@VincentMolinie would we need to revisit that implementation?

[runspired]

My guidance would be to get something that works 3.28->4.6

4.7+ changes are going to be a bit of a wade and the best course there is probably to just end support at 4.6 as ~4.11+ for data will come with this feature out of the box

[dwickern]

To support E-D 4+ we'll need to upgrade ember-cli. I've done most of the work already but I'd like to keep it separate from this PR. Could we get this merged to v6-development branch?

[knownasilya]

@patocallaghan what are your thoughts on that, should we make that breaking change for v5? v5 doesn't seem like it's ready for release without this PR...

[dwickern]

v5 works well for me with ED 3.27. What about this support matrix:

MF v5 for ED versions [3.13, 3.28) MF v6 for ED versions [3.28, 4.6]

[patocallaghan]

@knownasilya @dwickern yep those versions sound reasonable 👍

[knownasilya]

Released v5.0.0-beta.9 today. If this looks good, I can release a v5 later this week if everyone is fine with that.

[knownasilya]

v5 is out

[dwickern]

What can I do to help get this merged? I know it's a big change and difficult to review. I'll be happy to walk through the changes over a screen share.

Regarding test failures- the tests pass against E-D 3.28 (the main CI / Tests). Tests fail against older versions as they are no longer supported. It will take an ember-cli upgrade to get this working on release/beta/canary, which I didn't want to include in this PR.

[knownasilya]

@dwickern I wasn't aware this was done, since you kept adding commits 😄

[MelSumner]

@dwickern @deanmarano @runspired @knownasilya would love to help folks coordinate, this is a blocker for some folks at work who are trying to upgrade, so pinging y'all so you can all talk. 👍

[knownasilya]

I have it on my calendar to review this week.

[knownasilya]

@dwickern back to you

[dwickern]

All breaking changes:

1. Requires ember-data 3.28

2. changedAttributes now includes a deep copy of the fragment state in the form [oldFragment, newFragment]

3. typeKey as a function (#430) is not supported in this release

4. Fragments no longer fire unnecessary property change notifications when pushing data

   For example:

   // models/person.js
   export default class Person extends Model {
    @fragment('name') name;
   
    @computed('name') // <-- missing dependent keys
    get fullName() {
      return `${this.name.first} ${this.name.last}`;
    }
   }
   
   // models/name.js
   export default class Name extends Fragment {
     @attr('string') first;
     @attr('string') last;
   }
   

   The fullName computed property incorrectly depends on 'name'. In model-fragments 5.x, updating the first attribute via push would fire a change notification for 'first' as expected but also for 'name'. To fix this you should properly list the dependent keys. In this case change @computed('name') to @computed('name.{first,last}').

   Note that this only affects legacy computed properties and only when pushing data via store.push, store.pushPayload, model.save response, etc. Calling person.set('name.first', '...') did not (and still does not) fire a property change notification for 'name'.

   (credit to @richgt for finding this)

[knownasilya]

v6.0.0 is out, supports 3.28 - 4.4. See the readme for compatibility details. Also https://github.com/adopted-ember-addons/ember-data-model-fragments/pull/417#issuecomment-1546760562 ☝🏼