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

fix(rum-explorer): conversion specs should operate on facets, not on raw properties #570

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

trieloff
Copy link
Contributor

No description provided.

@aem-code-sync aem-code-sync bot temporarily deployed to fix-conversion-definition June 13, 2024 20:11 Inactive
Copy link

aem-code-sync bot commented Jun 13, 2024

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

@@ -768,6 +768,8 @@ describe('DataChunks.hasConversion', () => {
checkpoint: ['top'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we write the test something like

const spec = {
      facetOne: ['top'],
      facetTwo: ['hidden'],
    };
    d.addFacet('facetOne', (bundle) => bundle.events.map((e) => e.checkpoint));
    d.addFacet('facetTwo', (bundle) => bundle.events.map((e) => e.target));

This way it would be more clear that conversions work with facets not bundle events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

@@ -562,11 +562,20 @@ export class DataChunks {
hasConversion(aBundle, filterSpec, combiner) {
const existenceFilterFn = () => true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this operates with facets - no need to have a different function then filterBundles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

chore(rum-explorer): test that reflects how conversion works - facets…
@aem-code-sync aem-code-sync bot temporarily deployed to fix-conversion-definition June 14, 2024 14:17 Inactive
@trieloff trieloff merged commit 467588f into main Jun 14, 2024
2 of 3 checks passed
@trieloff trieloff deleted the fix-conversion-definition branch June 14, 2024 14:22
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.

None yet

2 participants