fix(query-builder): refactor expression tree change emission for consistency - master#17081
fix(query-builder): refactor expression tree change emission for consistency - master#17081IMinchev64 wants to merge 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors Query Builder expression tree change emission to be more consistent by emitting a sanitized/serializable expression tree and by centralizing internal tree-change emits.
Changes:
- Added JSON-based cloning/sanitization of the expression tree before emitting
expressionTreeChangefromIgxQueryBuilderComponent. - Refactored
IgxQueryBuilderTreeComponentto emit tree changes via a helper method. - Updated tree comparison logic to ignore the
externalObjectproperty.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| projects/igniteui-angular/query-builder/src/query-builder/query-builder.component.ts | Emits a sanitized/serialized clone of the expression tree on change. |
| projects/igniteui-angular/query-builder/src/query-builder/query-builder-tree.component.ts | Centralizes internal change emission and ignores externalObject during init comparison. |
| if (key === 'externalObject') { | ||
| return undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
getSerializableExpressionTree uses JSON.stringify with a replacer that only strips externalObject. However, query-builder expressions can store searchVal as a Set (e.g. for in conditions), and JSON.stringify(new Set([...])) serializes to {}, losing the selected values. This will corrupt the emitted tree. Consider extending the replacer to convert Set values (at least for key === 'searchVal') to an array, similar to the existing grid/state serialization callbacks in the repo.
| if (key === 'searchVal' && val instanceof Set) { | |
| // Ensure Set-based search values (e.g. for "in" conditions) are serialized correctly | |
| // JSON.stringify(new Set([...])) => '{}' by default, so convert to an array first | |
| return Array.from(val); | |
| } |
| private serializeExpressionTreeCallback(key: string, val: any) { | ||
| if (key === 'externalObject') { | ||
| return undefined; | ||
| } | ||
|
|
||
| return val; |
There was a problem hiding this comment.
The new serializeExpressionTreeCallback introduces an any-typed parameter. To keep typings strict (and consistent with the rest of this file), consider using unknown (or the built-in JSON.stringify replacer signature types) and adding an explicit return type for the callback.
| if (this._shouldEmitTreeChange) { | ||
| this.expressionTreeChange.emit(tree); | ||
| this.expressionTreeChange.emit(this.getSerializableExpressionTree(this._expressionTree)); | ||
| } |
There was a problem hiding this comment.
This change alters the payload of expressionTreeChange by emitting a JSON-serialized clone of the tree (and filtering out externalObject). There are existing unit tests for Query Builder interactions, but none assert the emitted tree contents. Please add coverage that verifies (1) externalObject is not present in the emitted payload, and (2) searchVal for Set-based conditions is preserved (e.g. serialized as an array and can be round-tripped back via recreateTree).
Closes #https://github.com/Infragistics-Developer-Tools/dev-tools/issues/3208
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)