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

[WIP] feat: minimal plugin system PoC #135

Closed
wants to merge 29 commits into from
Closed

[WIP] feat: minimal plugin system PoC #135

wants to merge 29 commits into from

Conversation

ramboz
Copy link
Collaborator

@ramboz ramboz commented Oct 18, 2022

Use case

As a developer, I'd like to have a minimal plugin system so that I can organize common features into individual opt-in plugins that I can re-use across projects.

Driving principles

  • Protection of the loading sequence (Eager, Lazy, Delayed)
  • Separation of concerns
  • Increase code testability
  • Keep PIS score at 100

Technical

  • plugins can be loaded via the await withPlugin(<path>, <options>) method
  • protect the loading phases by moving the logic inside lib-franklin.js
  • Offer pre/post hooks for the Eager/Lazy/Delayed phases so plugins can inject logic at the most appropriate moment in the loading lifecycle
  • pre/post hooks get the options object as well as a map of plugins that expose an API. For instance
    // this isn't publicly exposed and only available to direct importers
    export function aMethodNotExposedInTheApi() {  }
    
    // this is publicly exposed and passed down in the pre/post hooks
    export const API = {
      aMethodExposedInTheApi: () => {  } 
    }
    
    // defining a running before the Eager phase
    export async function preEager(options, plugins) {
      // options.myOption
      // plugins.myPlugin.myMethod(…)
    }
  • Plugins can expose a limited API usable via const myApi = await withPlugin('./plugins/myPlugin.js', …);
  • Simple eager/lazy/delayed hooks in the project via the usual load* methods and a configurable delayed timeout:
    async function loadEager(doc, options) {  }
    async function loadLazy(doc, options) {  }
    async function loadDelayed(doc, options) {  }
    
    init({
      loadEager,
      loadLazy,
      loadDelayed,
      delayedDuration: 3000
    });
  • RUM, decoration and placeholders logic extracted as plugins
  • Leveraging <link rel="modulepreload"> for the plugins to speed up page load
  • All plugin methods are executed with a context that gives access to the main lib-franklin utils (via this)

The loading sequence will be:

  • init of all plugins
  • preEager of all plugins
  • loadEager for the project
  • postEager of all plugins
  • preLazy of all plugins
  • loadLazy for the project
  • postLazy of all plugins
  • configurable timeout3s
  • preDelayed of all plugins
  • loadDelayed for the project
  • postDelayed of all plugins

Performance comparison with main

Screen Shot 2022-10-26 at 11 38 27 AM

Screen Shot 2022-10-26 at 11 37 04 AM

Fix #137

Test URLs:

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

aem-code-sync bot commented Oct 19, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 19, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 19, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

head.html Outdated Show resolved Hide resolved
@ramboz
Copy link
Collaborator Author

ramboz commented Oct 19, 2022

Some other ideas to explore:

  1. Expose a chainable object, instead of individual methods:
    Franklin
      .withPlugin('./plugins/foo.js', {})
      .withPlugin('./plugins/bar.js', {})
      .init({
        loadEager,
        loadLazy,
        loadDelayed,
      });
  2. Alias pre* in the plugin to load*, so that plugins that don't need both pre and post hooks are closer to the project's APIs
  3. Add option to make some plugins "blocking" if they are needed in the eager phase, versus non-blocking (by default) if they are only needed in the lazy phase:
    await withPlugin('./plugins/foo.js', {});
    withPlugin('./plugins/bar.js', {});
    or maybe via options { isBlocking: true } or similar
  4. Add plugins pre-loading via Link headers

@adobe adobe deleted a comment from aem-code-sync bot Oct 19, 2022
@adobe adobe deleted a comment from aem-code-sync bot Oct 19, 2022
@adobe adobe deleted a comment from aem-code-sync bot Oct 19, 2022
@ramboz ramboz changed the title feat: basic plugin system PoC feat: minimal plugin system PoC Oct 20, 2022
@ramboz ramboz mentioned this pull request Oct 20, 2022
@aem-code-sync
Copy link

aem-code-sync bot commented Oct 21, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 21, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 21, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 21, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

Copy link
Contributor

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

I think this breaks the advanced RUM functionality (Intersection Observer)

scripts/plugins/rum.js Outdated Show resolved Hide resolved
scripts/scripts.js Outdated Show resolved Hide resolved
scripts/plugins/rum.js Outdated Show resolved Hide resolved
test/scripts/block-utils.test.js Outdated Show resolved Hide resolved
@aem-code-sync
Copy link

aem-code-sync bot commented Oct 24, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@adobe adobe deleted a comment from aem-code-sync bot Oct 24, 2022
@aem-code-sync
Copy link

aem-code-sync bot commented Oct 24, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 24, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 24, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 26, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 26, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Oct 26, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Nov 2, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Nov 2, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Nov 2, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Nov 2, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@ramboz
Copy link
Collaborator Author

ramboz commented Nov 2, 2022

@trieloff, @kptdobe I think I addressed all the concerns you had. Do you see anything else that would need some love here?

I don't want to spend too much time working on bumping test coverage if you still have concerns with the underlying proposal.

@adobe adobe deleted a comment from aem-code-sync bot Nov 2, 2022
@adobe adobe deleted a comment from aem-code-sync bot Nov 2, 2022
@adobe adobe deleted a comment from aem-code-sync bot Nov 2, 2022
@adobe adobe deleted a comment from aem-code-sync bot Nov 2, 2022
@aem-code-sync
Copy link

aem-code-sync bot commented Nov 3, 2022

Page Score PSI Audit Google
/ SI FCP LCP TBT CLS PSI

@ramboz ramboz changed the title feat: minimal plugin system PoC [WIP] feat: minimal plugin system PoC Nov 4, 2022
@trieloff
Copy link
Contributor

trieloff commented Nov 4, 2022

Why don't we try this in a project and see if the benefits of maintainability pan out?

@ramboz
Copy link
Collaborator Author

ramboz commented Nov 7, 2022

@meryllblanchet Maybe something to test with one of your projects that are in the pipeline?

@meryllblanchet
Copy link
Collaborator

Sorry @ramboz I missed that notification - definitely, let's try this at customer project level first!

@ramboz
Copy link
Collaborator Author

ramboz commented Sep 6, 2023

Closing in favour of a more lightweight version in #254

@ramboz ramboz closed this Sep 6, 2023
omprakashgupta1995 referenced this pull request in WWWPiramalFinanceCOM/piramalfinance Jul 2, 2024
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.

Minimal plugin system
4 participants