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

issue #2960 shared-component-properties add support for lazy bindings #2962

Conversation

adamcin
Copy link
Contributor

@adamcin adamcin commented Oct 8, 2022

This is a hack approach to resolving #2960 in a way that doesn't force a significant change in the supported AEM version.

NOTE: Please see #2964 for non-hacky approach (against the 6.0.0 branch) that bumps the minimum AEM version to 6.5.7 and uses LazyBindings types properly.

Yet to be tested in AEM 6.4, 6.5 < SP7, 6.5 >= SP7, and Cloud Service.

The benefits of this approach:

  1. Should significantly improve page rendering performance in more recent versions of AEM 6.5 and Cloud Service where SCP is enabled.
  2. Avoids a rather drastic increase of the minimum supported AEM version for SCP from 6.4.0 to 6.5.7.
  3. Uses of reflection should be easy to prune without major refactoring when the minimum supported version of the project is raised past 6.5.7.

@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #2962 (b560d2a) into master (b881c90) will increase coverage by 0.05%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #2962      +/-   ##
============================================
+ Coverage     54.29%   54.34%   +0.05%     
- Complexity     5635     5660      +25     
============================================
  Files           756      756              
  Lines         30838    30895      +57     
  Branches       3984     3983       -1     
============================================
+ Hits          16744    16791      +47     
- Misses        12542    12549       +7     
- Partials       1552     1555       +3     
Impacted Files Coverage Δ
...aredComponentPropertiesBindingsValuesProvider.java 75.96% <82.05%> (+7.21%) ⬆️
...ties/shared/impl/SharedPropertiesRequestCache.java 94.11% <100.00%> (+0.36%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@adamcin adamcin force-pushed the feature/issue-2960-scpbvp-support-lazy-bindings branch 5 times, most recently from c8b7c08 to 5f7fc40 Compare October 8, 2022 19:35
@adamcin
Copy link
Contributor Author

adamcin commented Oct 8, 2022

Ready for review. @davidjgonzalez @HitmanInWis @joerghoh

@joerghoh
Copy link
Contributor

joerghoh commented Oct 9, 2022

This is a real hack[TM].

@adamcin I appreciate the effort you put into this PR to make it somehow work, but I am not sure if this is worth the effort. Maybe we should just wait until ACS AEM Commons drops support for older versions of AEM than AEM 6.5 SP7?

@davidjgonzalez Any plans to cut the support older SPs?

@kwin
Copy link
Contributor

kwin commented Oct 9, 2022

I would suggest requiring 6.5.x for ACS AEM Commons 6

@davidjgonzalez
Copy link
Contributor

davidjgonzalez commented Oct 21, 2022

@joerghoh @kwin im all on board with 6.5.x+ for AEM 6.0.0 ... i wouldnt even be adverse to specifying a higher level SP if it provides us more flexibility. One that that comes to mind is if AEM 6.5.x started supporting OSGi R7, which IIRC would let us move fully off Felix SCR without breaking OSGi properties that have - in them (which would be a big back-breaking change for alot of those older services). Havent taken the time to dig into R7 with AEM 6.5 tho to understand where that sits.

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

Successfully merging this pull request may close these issues.

4 participants