Skip to content

Conversation

RyanMG
Copy link
Contributor

@RyanMG RyanMG commented May 31, 2025

#3928

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

@amcclain
Copy link
Member

amcclain commented Jun 2, 2025

  • I vote for us adding StoreRecord.isDirty remains true even if all values edited back to unmodified state #3927 to the scope of the change and looking to get them both done together. This change would actually be more accurate, in that it's doing the direct field-wise value comparison between committed and current data, but then the new getter added here could conflict with e.g. StoreRecord.isDirty (new StoreRecord.modifedValues could be {} while isDirty == true. We don't ever want them to actually be out of sync, and ideally this method could early out if !isDirty, presumably returning {} as a signal that there are no modified values.
  • Should the new getter be modifiedData, matching data and committedData? Or is the difference in naming useful?
  • The ticket had called for us to return a list of modified field names. Returning an object with the fields and their values seems by definition more useful. I don't recall the exact use case that prompted the ask for the field list - but with this getter it's trivial to create that list at the app level. Let's keep it as-is, and if there's some clear reason to do so, we can always add another getter that builds the list off of here, to save apps the trouble.

@amcclain
Copy link
Member

amcclain commented Jun 2, 2025

I should add - re the fix for #3927 - I expect we'll need to look at where the store is applying these modifications, and have it do a new comparison and potentially "reset" data = committedData if it detects they are again the same. Hopefully we have a straightforward (I won't say "easy") path to doing that. If we get held up, we can of course revisit.

@RyanMG
Copy link
Contributor Author

RyanMG commented Jun 3, 2025

@amcclain I will pick up that second ticket and work on it as part of this PR (soon)

…t equals - ensures changing then reverting a change shows the grid as clean
@RyanMG
Copy link
Contributor Author

RyanMG commented Jun 5, 2025

@amcclain Quick update to review. I see this working in the toolbox example, but it may be simpler than what is actually needed here.

@RyanMG RyanMG force-pushed the 3928-store-modified-fields-getter branch from d5e16f5 to d471485 Compare June 5, 2025 22:41
@@ -72,7 +73,7 @@ export class StoreRecord {

/** True if the StoreRecord has been modified since it was last committed. */
get isDirty(): boolean {
return this.committedData && this.committedData !== this.data;
return this.committedData && !equal(this.committedData, this.data);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little concerned about the performance implications of this. For grids with many StoreRecords and wide StoreRecord this is an expensive operation.

@@ -705,7 +705,7 @@ export class Store extends HoistBase {
/** True if the store has changes which need to be committed. */
@computed
get isDirty(): boolean {
return this._current !== this._committed;
return !equal(this._current, this._committed);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is a whole "normalization" step (see RecordSet.normalize) that is supposed to do this evaluation after every update to _current and _committed and keep them pointer equivalent. the fact that the getter is computed, might mean that this is an efficient way to have the same outcome, although I will say that one benefit of the normalization step is that it just frees up memory after an edit gets committed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants