Add component diffing on event responses.

[richard-to]

This change is gated by a flag called enable_component_tree_diffs. When enabled, the full component tree will not be returned in event responses. Instead only the changed component fields will be returned to the frontend. The frontend will then apply the diff to update its copy of the component tree.

A few notes:

• Wasn't sure how to add unit tests for typescript. Is there a framework we're using?
• I'd like to run the playwright tests with the diff flag enabled and without it enabled (right now it's enabled by default in the test). Haven't looked into this too much yet. Seems like there's a way to do it
  • Passing in a different config but that would lead to a lot of duplication (so probably rule this out)
  • Seems like we could pass in some environment variable to enable/disable the flag. Then we'd need to update the CI to run the tests twice.

Ref #62, #84

[wwwillchen]

Here's an example for a TS unit test:

mesop/mesop/web/src/dev_tools/dev_tools_spec.ts

Line 4 in a795a41

import {EditorService} from '../services/editor_service';

mesop/mesop/web/src/dev_tools/BUILD

Lines 28 to 43 in a795a41

	ng_test_library(
	name = "dev_tools_tests_lib",
	srcs = glob(["*_spec.ts"]),
	deps = [
	":dev_tools",
	"//mesop/web/src/editor",
	"//mesop/web/src/services",
	],
	)
	ng_web_test_suite(
	name = "unit_tests",
	deps = [
	":dev_tools_tests_lib",
	],
	)

[wwwillchen]

Re: running w/ tests on and off - I think the environmental variable idea would work, you can also look at https://playwright.dev/docs/test-parameterize#introduction for some other ideas of how to parameterize test.

Also, I'd recommend adding some e2e tests with reasonably complex trees, multiple levels in the tree, different types of components & mutations (e.g. adding/deleting children) because most of our e2e tests are quite simple (usually 1 component with minimal hierarchy and mutations).

[wwwillchen]

Left some comments. Overall, this is looking quite promising - great work!

[wwwillchen]

Let's try to avoid copying the proto, which can be quite expensive.

I think we can solve this by modifying Shell's onRender function (

mesop/mesop/web/src/shell/shell.ts

Line 67 in a795a41

onRender: (rootComponent, componentConfigs) => {

) with:

this.changeDetectorRef.detectChanges();

We do this for markdown:

mesop/mesop/components/markdown/markdown.ts

Line 32 in a795a41

this.changeDetectorRef.detectChanges();

angular docs: https://angular.io/api/core/ChangeDetectorRef#detectchanges

[richard-to]

I tested this out. But I was getting weird behavior in the chat widget case. The streaming text wasn't showing updates as the text was being generated. So I kept the current copy behavior for now. I'll try again later.

[richard-to]

Re: running w/ tests on and off - I think the environmental variable idea would work, you can also look at https://playwright.dev/docs/test-parameterize#introduction for some other ideas of how to parameterize test.

Also, I'd recommend adding some e2e tests with reasonably complex trees, multiple levels in the tree, different types of components & mutations (e.g. adding/deleting children) because most of our e2e tests are quite simple (usually 1 component with minimal hierarchy and mutations).

Thanks for the pointer to the typescript tests. That worked for me.

---

Haven't got a chance to look into the Playwright flag stuff yet. Also haven't added more complex e2e examples, but agreed that it would be good to have them. Will get to those later this week.

[wwwillchen]

I don't think this method is being called anywhere? I tried running your PR locally, but I don't actually see any component diffs happening, I think because this method isn't actually being used.

[richard-to]

Doh, good catch. Looks like a regression when I added the reset_nodes method. It was supposed to call set_previous_node_from_current_node there.

[wwwillchen]

I patched your CL but I still don't see any component diffs. If you check the Chrome DevTools console logs, the protos received from the server are logged there. I don't see any component diffs in the proto.

[richard-to]

Ok, I think I got it working now 🤞🏾. Turns I didn't catch it because I was manually testing on the chat app which does the streaming response. But with the non-streaming response events it was not returning a diff. So basically the diff was not working on the first loop since previous node was not being set.

So that's the one thing I haven't figured out yet. I had to add back the original trace_mode loop code rather than existing early. However, I'm not sure why that works. I'm probably missing something pretty subtle.

[wwwillchen]

SGTM, we can debug this some more later.

[richard-to]

Added an e2e test to better test more complex diffing scenarios.

[wwwillchen]

LGTM - great job!