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.

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

perf(template-compiler): static-optimize on event listener object #4468

Merged
merged 22 commits into from
Sep 4, 2024

Conversation

cardoso
Copy link
Contributor

@cardoso cardoso commented Aug 20, 2024

Details

I'm still getting familiar with the codebase. Hopefully this makes sense. ๐Ÿคž

Closes #4467

Does this pull request introduce a breaking change?

  • ๐Ÿ˜ฎโ€๐Ÿ’จ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ๐Ÿคž No, it does not introduce an observable change.

GUS work item

W-16535749

@cardoso cardoso force-pushed the optimize-on branch 2 times, most recently from ce175cc to 7e0e6b7 Compare August 20, 2024 14:16
@nolanlawson
Copy link
Collaborator

Thanks a bunch for this PR!! Just wanted to let you know what we're a little slammed, but I will take a look this week. ๐Ÿ‘€

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

This is looking really great in general! Most of the snapshots look exactly right, and the Karma tests are still passing.

The only issue I found is the edge case of binding to variables defined within for:each loops. We need to handle those so that re-defining the event listeners works correctly (see comment above).

I also added some quick snapshot tests just to confirm that the entire on object is memoized when there are multiple event listeners on the same element.

Thanks a bunch for this PR!! Much appreciated. ๐Ÿ™

@nolanlawson
Copy link
Collaborator

Also, this PR seems to have just nudged us below the 95% target code coverage threshold. Opened a PR that should hopefully help bump it back up: #4482

@cardoso
Copy link
Contributor Author

cardoso commented Aug 24, 2024

@nolanlawson the test in #4480 is very helpful. Thanks!

PS: I copied here in order to validate locally since it wasn't merged yet.

@cardoso cardoso force-pushed the optimize-on branch 3 times, most recently from 074abba to 7044028 Compare August 24, 2024 10:03
Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Looking great to me. I'd just love to see another Karma test added and then I'd say :shipit:

We should get #4480 merged first to avoid merge conflicts.

Thank you very much!! ๐Ÿ™‡

@nolanlawson
Copy link
Collaborator

/nucleus test

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

Thank you!

@nolanlawson
Copy link
Collaborator

/nucleus test

@nolanlawson
Copy link
Collaborator

nolanlawson commented Aug 30, 2024

The improvement is not as much as I'd hoped, but it's definitely there! Looks to be ~0.5%-6% depending on the benchmark.

Screenshot 2024-08-30 at 1 56 26 PM

Screenshot 2024-08-30 at 2 25 47 PM

@cardoso cardoso force-pushed the optimize-on branch 5 times, most recently from aa4ff48 to 9b3ae82 Compare September 1, 2024 09:35
Copy link
Contributor Author

@cardoso cardoso left a comment

Choose a reason for hiding this comment

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

@nolanlawson here's the test for listeners with mixed local & non-local scopes in a single element.

I'll need to send my computer for repair this week, so please take over if I missed anything.

Thank you!

@nolanlawson
Copy link
Collaborator

/nucleus test

@nolanlawson
Copy link
Collaborator

Thanks again! This is fantastic work. ๐Ÿฅ‡

@nolanlawson nolanlawson merged commit 0985661 into salesforce:master Sep 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static-optimize on event listener object
3 participants