search

emberjs / data

A lightweight reactive data library for web applications. Designed over composable primitives.
https://api.emberjs.com/ember-data/release
Other
3.03k stars 1.33k forks source link

deleteRecord does not set hasDirtyAttributes to true #9053

Open jayseo5953 opened 12 months ago

jayseo5953 commented 12 months ago

Reproduction

Please provide one of the following:

Description

Versions

runspired commented 12 months ago

This was fixed in later versions. Broad strokes hasDirtyAttributes responding true for isDeleted is a mistake we don't want to carry forward to the replacement for @ember-data/model but it is one we intend to support until then.

If @fivetanley or @richgt wants to get this change into 4.4 then it shouldn't be hard to backport.

jayseo5953 commented 12 months ago

@runspired sorry do you mean hasDirtyAttributes being set to true by deleteRecord is a mistake? the document says otherwise tho https://api.emberjs.com/ember-data/4.4/classes/Model/properties/isDeleted?anchor=isDeleted

runspired commented 12 months ago

@jayseo5953 yes, it was a mistake that we did that is what I mean. hasDirtyAttributes should mean only "do I have dirty attributes" not "do I have dirty attributes or am I deleted".

Newer cache APIs allow us to differentiate these things and also provide access to whether relationships are dirty. We should likely have a single isDirty check that is a macro across cache.isDeleted / cache.isNew / cache.hasChangedAttrs / cache.hasChangedRelationships

jayseo5953 commented 12 months ago

@runspired right ok thanks for confirmation. Maybe the doc needs editing then?

runspired commented 12 months ago

@jayseo5953 no the docs are right and its a bug that in 4.4 it doesn't work that way. There's two different points being made here:

1) that there is a bug in 4.4 that is fixed in later releases that maybe we should backport to 4.4 2) that the bug is due to us supporting an undesirable behavior which we want to change in the future

I only mention (2) because for folks converting from @ember-data/model to @warp-drive/schema-record the 1:1 migration path will be to check both [STATE].isDeleted and [STATE].hasChangedAttrs, and to a certain degree its probably better to bake the assumption that you need to check both into your code in advance.

jayseo5953 commented 12 months ago

@runspired would you be able to point me to the closest version that has the fix? if it was fixed in minor versions

runspired commented 12 months ago

@jayseo5953 according to git blame the fix was part of this PR https://github.com/emberjs/data/pull/8849 (I recalled it being older than this but alas). That PR is currently part of 5.3, but we intend to port its work back to the 4.12 LTS

jayseo5953 commented 12 months ago

@runspired thank you!