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

AngularLoader - Emit warning when setModules method is used. #20628

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

colemanw
Copy link
Member

Overview

Emit warning when extensions do something that could potentially cause problems with other extensions.

It can cause conflicts with other extensions who have added modules.
@civibot
Copy link

civibot bot commented Jun 16, 2021

(Standard links)

@civibot civibot bot added the master label Jun 16, 2021
@totten
Copy link
Member

totten commented Jun 17, 2021

I'm good with this change, with a caveat.

The situation is that we are changing recommended usage/protocol.

  • Old protocol/Unique loaders:new AngularLoader()... ->setModules(['my','list'])
  • New protocol/Shared loader: Civi::service('angularjs.loader')...->addModules(['my','list'])

(Speaking of which.... on r-doc, this needs an update for civicrm-dev-docs/docs/framework/angular/loader.md.)

Marking setModules() as deprecated has the effect of emitting a warning on everything that uses the old protocol, which is good. Because AFAICS everything that uses the old protocol would be better served by the new protocol.

OTOH, the method setModules() isn't bad or complex. There are hypothetically valid ways to use setModules() in the new protocol (e.g. filtering/substituting - setModules(applyMyFilter(getModules()))). But that's so obscure/speculative... it's really not worth spending any time on it unless someone actually needs that case.

So I'm +1 to mark this deprecated, owing to the real-world/foreseeable impact. But if (in 3 months time) somebody comes around with a good/concrete scenario, then setModules() could be rehabilitated.

@totten totten merged commit 4813dd0 into civicrm:master Jun 17, 2021
@totten totten deleted the deprecateAngular branch June 17, 2021 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants