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

[REF] Cleanup Ang modules in core to follow conventions #19052

Merged
merged 2 commits into from Nov 26, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Nov 25, 2020

Overview

Each Angular module has a "definition" (typically in an .ang.php file) which consists of a small amount of information about where the html, js & css files are kept. Our goal is to cache Angular module definitions for performance, which means the data must be static. Historically .ang.php files were allowed to return an array of "settings" which could contain dynamic information (e.g. the current user's permission levels). That can't be cached, so instead modules can declare "settingsFactory" which is the name of a function which will return the settings.

Attention Extension Authors: You can refactor your own extensions simply by moving the settings array into a function.

Before

  • crmAttachment module loaded settings at runtime.

After

  • crmAttachment module loads settings via factory. Other references to settings removed.

Technical Details

This gets us another step closer to being able to autoload Angular modules in core and cache the list of modules.

Also renamed module statuspage to crmStatusPage to match file name crmStatusPage.ang.php.

@civibot
Copy link

civibot bot commented Nov 25, 2020

(Standard links)

@civibot civibot bot added the master label Nov 25, 2020
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@totten
Copy link
Member

totten commented Nov 25, 2020

I haven't r-run, but r-code looks good. 👍

@seamuslee001
Copy link
Contributor

@colemanw test seems to relate

Civi\Angular\ManagerTest::testGetModules
Expect to find at least one setting
Failed asserting that false is true.

@colemanw
Copy link
Member Author

Thanks @seamuslee001 I fixed the test.

We are moving toward the use of settingsFactory and away from settings for angular modules.
This would allow the list of modules to be cached as it would only contain static metadata.
@eileenmcnaughton
Copy link
Contributor

Since code changes involved the status screen & export I logged into the test site & checked those screens still loaded & responded to basic clicking - which they did. This seemed to be pending only r-run so I'm merging

@agileware-justin
Copy link
Contributor

https://lab.civicrm.org/extensions/angularprofiles/-/issues/5 - org.civicrm.angularprofiles

And also relevant, https://lab.civicrm.org/extensions/angularprofiles/-/issues/3 - an we Deprecate This Extension?

@colemanw
Copy link
Member Author

@agileware-justin [c]an we Deprecate This Extension?

Probably. But in the meantime I went ahead and fixed the notice: https://lab.civicrm.org/extensions/angularprofiles/-/merge_requests/2

@agileware-justin
Copy link
Contributor

Thanks again @colemanw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants