Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: data items nested templates #2639

Merged
merged 5 commits into from
Dec 20, 2024
Merged

fix: data items nested templates #2639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a267446
fix: data items nested templates
chrismclarke Dec 18, 2024
16903c8
Merge branch 'master' into fix/data-items-nested-templates
jfmcquade Dec 18, 2024
6ff0da8
fix: container recreate on template name change
chrismclarke Dec 19, 2024
fd346d6
Merge branch 'master' into fix/data-items-nested-templates
chrismclarke Dec 19, 2024
462cebe
Merge branch 'master' into fix/data-items-nested-templates
esmeetewinkel Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/shared/components/template/components/data-items/data-items.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,6 @@ export class DataItemsService {
// HACK - still want to be able to use localContext from parent rows so copy to child processor
processor.templateRowMap = JSON.parse(JSON.stringify(templateRowMap));
await processor.processContainerTemplateRows();
return processor.renderedRows;
return processor.renderedRows();
}
}
2 changes: 1 addition & 1 deletion src/app/shared/components/template/components/data-items/data-items.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export function updateItemMeta(
}

// Apply recursively to ensure item children with nested rows (e.g. display groups) also inherit item context
if (r.rows) {
if (r.rows && r.type !== "data_items" && r.type !== "items") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might benefit from an explanatory comment on why data_items and items rows are excluded

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in #2647

r.rows = updateItemMeta(r.rows, itemData, dataListName);
}

Expand Down
7 changes: 4 additions & 3 deletions src/app/shared/components/template/services/instance/template-row.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Injector } from "@angular/core";
import { Injector, signal } from "@angular/core";
import { FlowTypes } from "src/app/shared/model";
import { getGlobalService } from "src/app/shared/services/global.service";
import { SyncServiceBase } from "src/app/shared/services/syncService.base";
Expand All @@ -9,6 +9,7 @@ import { mergeTemplateRows } from "../../utils/template-utils";
import { TemplateFieldService } from "../template-field.service";
import { TemplateTranslateService } from "../template-translate.service";
import { TemplateVariablesService } from "../template-variables.service";
import { isEqual } from "packages/shared/src/utils/object-utils";

/** Logging Toggle - rewrite default functions to enable or disable inline logs */
let SHOW_DEBUG_LOGS = false;
Expand All @@ -34,7 +35,7 @@ export class TemplateRowService extends SyncServiceBase {
/** List of overrides set by parent templates for access during parent processing */
/** Hashmap of all rows keyed by nested row name (e.g. contentBox1.row1.title) */
public templateRowMap: ITemplateRowMap = {};
public renderedRows: FlowTypes.TemplateRow[]; // rows processed and filtered by condition
public renderedRows = signal<FlowTypes.TemplateRow[]>([], { equal: isEqual }); // rows processed and filtered by condition

constructor(
private injector: Injector,
Expand Down Expand Up @@ -234,7 +235,7 @@ export class TemplateRowService extends SyncServiceBase {
const renderedRows = this.filterConditionalTemplateRows(
JSON.parse(JSON.stringify(processedRows))
);
this.renderedRows = renderedRows;
this.renderedRows.set(renderedRows);
log("[Rows Processed]", logName, { rows, processedRows, renderedRows });
return processedRows;
}
Expand Down
35 changes: 27 additions & 8 deletions src/app/shared/components/template/template-component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ export class TemplateComponent implements OnInit, AfterContentInit, ITemplateRow
this._row = row;
if (this.componentRef) {
log("[Component Update]", row.name, row);
this.componentRef.instance.row = row;
this.componentRef.setInput("row", row);
this.hackForceReprocessNestedTemplate();
} else {
log("[Component Create]", row.name, row);
}
Expand Down Expand Up @@ -130,7 +131,7 @@ export class TemplateComponent implements OnInit, AfterContentInit, ITemplateRow
// Depending on row type, either prepare instantiation of a nested template or a display component
switch (row.type) {
case "template":
return this.renderTemplateComponent(TemplateContainerComponent, row);
return this.renderTemplateComponent(row);
default:
const displayComponent = TEMPLATE_COMPONENT_MAPPING[row.type];
if (displayComponent) {
Expand All @@ -145,15 +146,12 @@ export class TemplateComponent implements OnInit, AfterContentInit, ITemplateRow
}

/** Create and render a nested template component */
private renderTemplateComponent(
component: typeof TemplateContainerComponent,
row: FlowTypes.TemplateRow
) {
private renderTemplateComponent(row: FlowTypes.TemplateRow) {
const viewContainerRef = this.tmplComponentHost.viewContainerRef;
const componentRef = viewContainerRef.createComponent(component);
const componentRef = viewContainerRef.createComponent(TemplateContainerComponent);
// assign input variables (note template name taken from the row's value column)
componentRef.instance.parent = this.parent;
componentRef.instance.row = row;
componentRef.setInput("row", row);
componentRef.instance.name = row.name;
// assign templatename input using signal
componentRef.setInput("templatename", row.value);
Expand All @@ -169,4 +167,25 @@ export class TemplateComponent implements OnInit, AfterContentInit, ITemplateRow
componentRef.instance.row = row;
this.componentRef = componentRef;
}

/**
* If the current template is generated as a child of data_items then updates to
* parent variables will not propagate down. Force reprocessing to workaround
* See issue https://github.com/IDEMSInternational/open-app-builder/issues/2636
*/
private async hackForceReprocessNestedTemplate() {
if (this._row.type === "template") {
const componentRef = this.componentRef as ComponentRef<TemplateContainerComponent>;
if (componentRef.instance.parent) {
// HACK - if parent template changes name of nested child template instance fully
// recreate the template container
if ((componentRef.instance.templatename(), this._row.value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This use of the comma operator isn't clear to me.

I'm supposing that the signal value is being accessed here to ensure that any reactive side-effects (including ultimately updating this._row.value) have been triggered before accessing the value, is that right? Or else, to achieve the same result of not accessing the value until any signal-based changes have propagated, I can see how this line could be establishing a reactive dependency between this function and the source signal that wouldn't otherwise be there.

If that's right, might it be clearer to access the signal outside of the if statement and add a comment to explain the dependency being established? I think my confusion is mostly down to unfamiliarity with the comma operator, I can see how this could be an established pattern for making sure the first expression has evaluated and any side-effects propagated before evaluating the first expression, but I wouldn't have expected this to be required when using signals.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, that was meant to be a !== operator, although when I change it the debug cases fails to update when parent local variable (as was actually expected, and confusing why it was passing)

So I think for now we'll just have to stick with the heavier condition to always fully recreate when the parent of a child template changes (first part of componentRef.instance.templateName being truthy), and try to improve a more finegrained reprocessing when refactoring the templating system

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in #2647

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the visual-tests failing, I see this happening generally example failed action

[1220/175851.640539:FATAL:zygote_host_impl_linux.cc(117)] No usable sandbox! Update your kernel or see 
https://chromium.googlesource.com/chromium/src/+/main/docs/linux/suid_sandbox_development.md for more information on developing with the SUID sandbox. If you want to live dangerously and need an immediate workaround, you can try using --no-sandbox.

so I think I'll give it a couple days to see if it's an issue resolved within github actions (could be an issue related to internal action runner updates), before looking more closely and trying to update the action/dependencies

this.componentRef.destroy();
this.renderTemplateComponent(this._row);
// TODO - test better ways to manage from container, confirming test cases from
// https://github.com/IDEMSInternational/open-app-builder/issues/2636
}
}
}
}
}
42 changes: 19 additions & 23 deletions src/app/shared/components/template/template-container.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,27 @@
<div>name: {{ name || "(undefined)" }}</div>
</div>
<!-- Template Component-->
<plh-template-component
*ngFor="
let row of templateRowService.renderedRows | filterDisplayComponent;
trackBy: trackByRow
"
class="row"
[row]="row"
[parent]="this"
[attr.data-name]="row.name"
[attr.data-type]="row.type"
data-debug-mode
></plh-template-component>
@for (row of templateRowService.renderedRows() | filterDisplayComponent; track row._nested_name) {
<plh-template-component
class="row"
[row]="row"
[parent]="this"
[attr.data-name]="row.name"
[attr.data-type]="row.type"
data-debug-mode
></plh-template-component>
}
</ng-template>

<!-- Normal Mode -->
<ng-template #noDebugger>
<plh-template-component
*ngFor="
let row of templateRowService.renderedRows | filterDisplayComponent;
trackBy: trackByRow
"
class="row"
[row]="row"
[parent]="this"
[attr.data-name]="row.name"
[attr.data-type]="row.type"
></plh-template-component>
@for (row of templateRowService.renderedRows() | filterDisplayComponent; track row._nested_name) {
<plh-template-component
class="row"
[row]="row"
[parent]="this"
[attr.data-name]="row.name"
[attr.data-type]="row.type"
></plh-template-component>
}
</ng-template>
7 changes: 2 additions & 5 deletions src/app/shared/components/template/template-container.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,15 +146,12 @@ export class TemplateContainerComponent implements OnInit, OnDestroy {
if (shouldProcess) {
if (full) {
console.log("[Force Reload]", this.name);
// ensure angular destroys previous row components before rendering new
// (note - will cause short content flicker)
this.templateRowService.renderedRows = [];
// allow time for other pending ops to finish
await _wait(50);
await this.renderTemplate(this.templatename());
} else {
await this.templateRowService.processRowUpdates();
console.log("[Force Reprocess]", this.name, this.templateRowService.renderedRows);
console.log("[Force Reprocess]", this.name, this.templateRowService.renderedRows());
for (const child of Object.values(this.children || {})) {
await child.forceRerender(full, shouldProcess);
}
Expand Down Expand Up @@ -203,7 +200,7 @@ export class TemplateContainerComponent implements OnInit, OnDestroy {
log("[Template] Rendered", this.name, {
template,
ctxt: { ...this },
renderedRows: { ...this.templateRowService.renderedRows },
renderedRows: { ...this.templateRowService.renderedRows() },
rowMap: this.templateRowService.templateRowMap,
});
// if a parent exists also provide parent reference to this as a child
Expand Down
Loading