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: introduce plugin system to offset non-essenial logic #348

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

ramboz
Copy link
Collaborator

@ramboz ramboz commented Jan 14, 2025

As discussed over Slack, we want to introduce a plugin system to offset some of the non-essential logic and reduce the core library's overall file size.

At a high level, we want:

  1. a minimalistic probing logic that checks the markup or page context for some "markers" that indicate additional optional logic is needed
  2. a loader script that pulls in the required script for the particular plugin
  3. the plugin should have a default export that gets the required context passed as input

Test Url: https://rum-plugin-system--helix-website--adobe.aem.live/?rum=on
PR against helix website: adobe/helix-website#754

Copy link

aem-code-sync bot commented Jan 14, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

@adobe-bot
Copy link
Collaborator

This PR will trigger a minor release when merged.

@ramboz
Copy link
Collaborator Author

ramboz commented Jan 14, 2025

This is a first draft that brings the code down to ~12.5kB.

@@ -64,99 +111,6 @@ function processQueue() {
}
}

function addCWVTracking() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to cwv.js

}, 2000); // wait for delayed
}

function addNavigationTracking() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to navigation.js

@@ -211,8 +165,6 @@ function getIntersectionObsever(checkpoint) {
if (!window.IntersectionObserver) {
return null;
}
activateBlocksMO();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved down addTrackingFromConfig

@@ -251,28 +203,6 @@ function addViewMediaTracking(parent) {
}
}

function addFormTracking(parent) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to form.js

modules/index.js Outdated Show resolved Hide resolved
Comment on lines -17 to -22
import {
addAdsParametersTracking,
addCookieConsentTracking,
addEmailParameterTracking,
addUTMParametersTracking,
} from './martech.js';
Copy link
Collaborator Author

@ramboz ramboz Jan 14, 2025

Choose a reason for hiding this comment

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

Moved to martech.js & onetrust.js

@ramboz ramboz requested a review from trieloff January 14, 2025 01:07
@trieloff
Copy link
Contributor

Custom plugins can be defined at the project level via window.RUM_PLUGINS

Let's make that a non-goal:

Custom plugins can be defined at the project level via window.RUM_PLUGINS

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.

That's a good start. I wonder if we can make the CWS plugin more elegant, so that it does not need to load two files after another. Maybe rollup the dependency.

modules/index.js Outdated Show resolved Hide resolved
modules/index.js Outdated Show resolved Hide resolved
modules/index.js Outdated Show resolved Hide resolved
modules/index.js Outdated Show resolved Hide resolved
modules/index.js Outdated Show resolved Hide resolved
modules/index.js Outdated Show resolved Hide resolved
modules/index.js Outdated Show resolved Hide resolved
plugins/onetrust.js Outdated Show resolved Hide resolved
@ramboz
Copy link
Collaborator Author

ramboz commented Jan 16, 2025

I tried a few ways to include the web-vitals lib directly in the cwv.js plugin to avoid another network roundtrip… but I'm hitting: aMarCruz/rollup-plugin-cleanup#16

So unless we stop cleaning up the output files in rollup, I think we'll be stuck having to load this separately

trieloff and others added 17 commits January 16, 2025 17:26
# [2.29.0](v2.28.0...v2.29.0) (2025-01-15)

### Features

* **loadresource:** add `allresources` feature flag that enables tracking all resources, not just same host ([7c536d7](7c536d7))
the target field was until now unused. this change uses it for the status code and in turn tracks all errored resources as `missingresource`
# [2.30.0](v2.29.0...v2.30.0) (2025-01-15)

### Features

* **missingresource:** report all resource error with http status code ([e238136](e238136))
# [2.31.0-beta.1](v2.30.0...v2.31.0-beta.1) (2025-01-16)

### Features

* handle pre-rendering ([bb11187](bb11187))
# [2.31.0-beta.2](v2.31.0-beta.1...v2.31.0-beta.2) (2025-01-16)

### Reverts

* Revert "feat: handle pre-rendering" ([6471af5](6471af5))
# [2.31.0-beta.3](v2.31.0-beta.2...v2.31.0-beta.3) (2025-01-16)

### Bug Fixes

* bugs and failing tests ([5c9adbc](5c9adbc))
* cross-browser compatibility ([1c21afd](1c21afd))
* set proper url base for plugins ([9726dc8](9726dc8))
* use proper fully qualified URLs for plugin path to avoid CORS ([c73eb31](c73eb31))

### Features

* allow external extension via window.RUM_PLUGINS ([4613b00](4613b00))
* inline web-vitals.js ([7d89e9f](7d89e9f))
* introduce minimal plugin system to offset non-essenial logic ([8642557](8642557))
* introduce minimal plugin system to offset non-essenial logic ([6066f0b](6066f0b))
# [2.31.0-beta.4](v2.31.0-beta.3...v2.31.0-beta.4) (2025-01-16)

### Bug Fixes

* plugin path resolution ([f464652](f464652))
# [2.31.0-beta.5](v2.31.0-beta.4...v2.31.0-beta.5) (2025-01-17)

### Bug Fixes

* just forcing a release ([84ab687](84ab687))

export default function addVideoTracking({ context }) {
context.querySelectorAll('video').forEach(() => {
// TODO: Add video tracking
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just an empty placeholder for now

@trieloff trieloff self-requested a review January 20, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants