-
Notifications
You must be signed in to change notification settings - Fork 362
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: a minimal plugin and template system #254
Conversation
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin system is great, and much needed.
It provides a formal way to introduce add-ons to Franklin/EDS, in an optimal, easy, and non-intrusive way.
There are many features that would benefit from it:
- experimentation
- rum conversion tracking
- Martech integration (neutrino)
- omnivore
- etc...
*/ | ||
function runFunctionWithContext(fn, args, context) { | ||
return fn.toString().startsWith('function') | ||
? fn.call(context, ...args, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth to use .call
?
It feels a bit strange to have methods which are defined in aem-lib
(or lib-franklin
) in the this
object of a plugin function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind it was:
- simplify imports and given immediate access to all "core" functions
- allow for "overriding" core functions by modifying the context if needed, as opposed to imports which are read-only (think icons decoration via svg spriting for instance that could replace the regular
decorateIcons
) - simplify unit testing, since you don't have to inject mocked dependencies anymore
} | ||
|
||
// Register a new plugin | ||
add(id, config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that plugins could register themselves, instead of being explicitly registered in the scripts.js
as suggested in the docs. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a plugin to register itself, it means we need to load the plugin unconditionally and then let it decide itself if it triggers or not. I think this goes against the performance-first paradigm.
If you have a hefty preview overlay plugin for instance, and the plugin registers itself, that means you have to load a large JS file just to see the plugin not activate when the conditions aren't met (i.e. not preview, and likely lazy loaded)
@davidnuescheler @rofe @kptdobe @fkakatie Would love to have your thoughts on the current proposal to see how to fine-tune this. |
The current is:
|
And we also agreed to remove the |
Refactor the engine to adopt the new plugin system proposal in adobe/aem-boilerplate#254. BREAKING: - The new plugin API changes the high-level signature of the exported methods. All methods now follow a `*(document, options, context)` signature, and do not pass a `this` context anymore Test URLs: - https://main--aem-experience-decisioning-demo--ramboz.hlx.page/audiences/ - https://main--aem-experience-decisioning-demo--ramboz.hlx.page/campaigns/ - https://main--aem-experience-decisioning-demo--ramboz.hlx.page/experiments/
The PR adds the plugin system proposed in adobe/aem-boilerplate#254. It also updates the 2 used plugins to their latest versions and adds some of the missing lib-franklin.js methods that are expected to be there. Test URLs: - Before: https://main--mammotome--hlxsites.hlx.page/ - After: https://plugin-system--mammotome--hlxsites.hlx.page/
The PR adds the plugin system proposed in adobe/aem-boilerplate#254. It also updates a few lib-franklin.js methods that were missing, along with the blocks that are impacted, and the template loading is converted to the new system. Test URLs: - Before: https://main--moleculardevices--hlxsites.hlx.page/ - After: https://plugin-system--moleculardevices--hlxsites.hlx.page/
As discussed with @trieloff during our recent workshop, I'm adding the plugin system and the experimentation plugin to the website. ## Description 1. Adds the minimal plugin system proposal from adobe/aem-boilerplate#254 2. Adds the experimentation plugin from https://github.com/adobe/aem-experience-decisioning/tree/plugin-api 3. Instrument the existing `performance.js` as a plugin
The PR adds the plugin system proposed in adobe/aem-boilerplate#254 and updates the rum conversion instrumentation to leverage it. Test URLs: - Before: https://main--bitdefender--hlxsites.hlx.page/solutions/ - After: https://plugin-system--bitdefender--hlxsites.hlx.page/solutions/
@ramboz any update on this? |
@rofe We've tried it on a dozen websites so far, and it's working fine. But I remember discussions with @trieloff to try to simplify this and leverage more the |
Closing in favor of #374 |
Use case
As a developer, I want to be able to easily organize my code into different files, and execute some of the logic in each phase of the page load as needed.
In particular, I want a way to:
templates
that have some JS logicplugins
for more advanced features like experimentation, conversion tracking, tag management, etc.templates
andplugins
only when they are neededplugin
is loaded to avoid negatively impacting performances if it is not needed immediatelyTechnical Requirements
plugins
andtemplates
should use the same logic to reduce code duplicationplugins
andtemplates
should re-use the block loading approach to load both JS and CSS files, if applicableplugins
should offer a way to "override" core methods, for instance to add icon spriting on top of regular icon decorationwindow.hlx.*
namespaceProposal
Introduce 2 new namespaces on
window.hlx.*
:window.hlx.templates
add(id, url)
: register a new templateget(id)
: get the templateincludes(id)
: check if a template existswindow.hlx.plugins
add(id, config)
: add a new pluginget(id)
: get the pluginincludes(id)
: check if a plugin existsload(phase)
: load all plugins for the current phase (eager
,lazy
ordelayed
)run(method)
: runs the specified method on the plugins (loadEager
,loadLazy
orloadDelayed
)Plugins and templates would follow the same API:
export default function init(document, options)
: logic executed immediately when the plugin/template is loadedexport function loadEager(document, options)
: logic executed in theeager
phaseexport function loadLazy(document, options)
: logic executed in thelazy
phaseexport function loadDelayed(document, options)
: logic executed in thedelayed
phasewhere
this
is set to an execution context that contains mostlib-franklin.js
helper functions, so we can reduce imports in the plugins (makes it easier to mock in tests, and also avoids dependency cycles).Or the slightly more verbose version below for arrow functions:
export default (document, options, context) => {}
export const loadEager = (document, options, context) => {}
export const loadLazy = (document, options, context) => {}
export const loadDelayed = (document, options, context) => {}
ℹ️ All those exports are optional and are only executed if present.
Extracting a new
loadModule(name, cssPath, jsPath, args)
method that both theloadBlock
and the new plugin system use.Usage
Adding Templates
Add a new template to the project:
or the shorthand version:
or the bulk version:
or the the module approach that loads both js and css:
Adding Plugins
Add a new inline plugin to the project:
Add a regular plugin to the project that will always load (no
condition
):or the module approach that loads both js and CSS:
or the shorthand version:
Add a plugin that only loads in the lazy
phase
and also passes some additional options:Once plugins have been loaded via
window.hlx.plugins.load(phase)
, we can then run individual methods as required viawindow.hlx.plugins.run('loadEager')
, etc.Plugin Template
/plugins/qux.js
When using proper functions, thethis
variable is set to the context, so we can ignore the last parameter. This is mostly for backward compatibility with existing approaches that were passing it down that way until now, but also offers a slightly shorter API.Or with proper functions:
/plugins/qux.js
Examples
A barebone demo site built for this PR:
We have a few sites that are being instrumented with this:
We also do have a few plugins that already follow this API:
As well as a few template mechanisms that could leverage it:
Those could all be aligned on a consistent system.
Test URLs: