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

feat: add capability to patch block config at runtime #246

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

ramboz
Copy link
Collaborator

@ramboz ramboz commented Aug 11, 2023

We've had several cases in the past where we needed to override the default block config logic when loading blocks:

  • experimentations may load from a different folder name, or load a different js/css file in the same block folder
  • themes that were loading alternate CSS files for block variants

To make maintaining those downstream projects easier, I propose to directly add an extension mechanism so we have fewer modifications to the lib-franklin.js file.

Test URLs:

Sample usage: https://github.com/ramboz/franklin-experience-decisioning/blob/main/scripts/experience-decisioning/index.js#L359-L417

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 11, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 11, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 11, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link
Contributor

@rofe rofe left a comment

Choose a reason for hiding this comment

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

LGTM, apart from a few jsdoc typos.

The sampe usage link looks wrong, or maybe the repo is private?

scripts/lib-franklin.js Outdated Show resolved Hide resolved
@aem-code-sync
Copy link

aem-code-sync bot commented Aug 11, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@ramboz
Copy link
Collaborator Author

ramboz commented Aug 11, 2023

@rofe Right, forgot I had the repo as private. I opened it up now.

scripts/lib-franklin.js Outdated Show resolved Hide resolved
const original = { blockName, cssPath, jsPath };
return window.hlx.patchBlockConfig.reduce(
(config, fn) => (typeof fn === 'function' ? fn(config, original) : config) || config,
{ blockName, cssPath, jsPath },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not original to initialize the reduce?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was just to avoid someone modifying it inside the fn and potentially breaking the values.

Co-authored-by: Lars Trieloff <lars@trieloff.net>
@aem-code-sync
Copy link

aem-code-sync bot commented Aug 16, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 16, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@ramboz
Copy link
Collaborator Author

ramboz commented Aug 16, 2023

@trieloff @rofe Once this is merged, do I need to formally document this somewhere?

@rofe
Copy link
Contributor

rofe commented Aug 19, 2023

@trieloff @rofe Once this is merged, do I need to formally document this somewhere?

Ideally yes. If there isn't already, there should probably be an experimentation page in the developer section of www.hlx.live.

@ramboz
Copy link
Collaborator Author

ramboz commented Aug 21, 2023

@rofe This doesn't add experimentation, just an extension point for changing how block-related files (JS, CSS) are loaded.

I was wondering if extension points are documented somewhere and if I need to add this to it.

@ramboz ramboz merged commit a774acc into adobe:main Aug 25, 2023
2 checks passed
@ramboz ramboz deleted the extend-block-config branch August 25, 2023 21:51
@rofe
Copy link
Contributor

rofe commented Aug 29, 2023

This doesn't add experimentation
I know, I just thought experimentation would be a concrete use case to document how this extension point can be used.

I was wondering if extension points are documented somewhere and if I need to add this to it.
I don't think so, but might be a good idea to start one.

github-actions bot pushed a commit that referenced this pull request Nov 30, 2023
# [1.3.0](v1.2.2...v1.3.0) (2023-11-30)

### Bug Fixes

* attempt to fix release ([5bb8f73](5bb8f73))
* **build:** fix org in issues URL ([7226d84](7226d84))
* **build:** no double slashes in issues url ([1191064](1191064))
* **github:** increase default permissions for cleanup action ([f8ed051](f8ed051))
* **github:** restrict running of action to initial commit on main branch ([1a990c2](1a990c2))
* icons no more svgs ([#267](#267)) ([2d4cfa7](2d4cfa7))
* **lib:** update scripts/aem.js to aem.js@1.3.2 ([#266](#266)) ([2d61c6a](2d61c6a))
* rescope icon identifiers to avoid clashing references across icons ([#241](#241)) ([e8dc533](e8dc533)), closes [#235](#235)
* sampleRUM always for checkpoint to be called even if RUM not selected ([fcca39d](fcca39d))
* trigger release ([afbd201](afbd201))

### Features

* add capability to patch block config at runtime ([#246](#246)) ([a774acc](a774acc))
* enable listeners to all RUM events, not just the sampled ones ([871ede4](871ede4))
* use aem.js ([#255](#255)) ([f5ab4df](f5ab4df))
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants