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

Improve AngularJS performance with one-time binding for static strings #17050

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

colemanw
Copy link
Member

Overview

Improves the run speed of all Angular pages in CiviCRM by making use of the one-time binding feature for static strings being passed through ts().

Before

Angular pages slower

After

Angular pages faster

Technical Details

One-time binding prevents unnecessary $watch expressions on values that will never change, making the digest loop faster.
See https://docs.angularjs.org/guide/expression#one-time-binding

Comments

I also included a test case for strings in one-time binding expressions.

@civibot
Copy link

civibot bot commented Apr 10, 2020

(Standard links)

@civibot civibot bot added the master label Apr 10, 2020
Use one-time binding for all static strings being passed through ts().
This prevents unnecessary $watch expressions, making the digest loop faster.
See https://docs.angularjs.org/guide/expression#one-time-binding
@colemanw
Copy link
Member Author

@demeritcowboy my regex was a bit too conservative. I've updated it to cast a wider net, should still be excluding any expression that uses variable substitution. Thanks for reviewing :)

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • I have no data on performance increase but everything seems to work.
      • I also checked what happens if you have visited the page and then change the language whether this causes it to get "stuck" on the old binding but it uses the new lang ok.
        • There's a pre-existing issue if you use the language switcher while on an angular page but I think that's been logged elsewhere already, just can't find it. If I can't find it will create ticket.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • As an aside, I was also curious about the use of ts('HTML'). I think it's technically correct and best to leave it, just noting that the internet suggests that other languages use HTML, SQL, etc as-is and a quick look at the civi repo suggest the same, even for languages like Russian with a different alphabet. Just noting, since this PR is about performance.
    • [r-maint] PASS
    • [r-test] PASS

@colemanw colemanw merged commit 5d9d205 into civicrm:master Apr 11, 2020
@colemanw colemanw deleted the one-time-binding branch April 11, 2020 17:56
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.

2 participants