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

change collectbase to /.rum/ #443

Closed
wants to merge 1 commit into from
Closed

change collectbase to /.rum/ #443

wants to merge 1 commit into from

Conversation

dkuntze
Copy link

@dkuntze dkuntze commented Jan 10, 2025

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

This change merely adjusts the collectBaseURL to use the window.origin and thus not the rum.hlx.page domain.

Copy link

aem-code-sync bot commented Jan 10, 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

Copy link

aem-code-sync bot commented Jan 10, 2025

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

@dkuntze dkuntze marked this pull request as ready for review January 10, 2025 15:46
@dkuntze dkuntze requested review from kptdobe and trieloff January 10, 2025 15:48
Copy link
Contributor

@kptdobe kptdobe left a comment

Choose a reason for hiding this comment

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

Ok with the change but aem.js is coming from https://github.com/adobe/aem-lib which brings along the sampleRUM function from https://github.com/adobe/helix-rum-js. This is where the change needs to happen!

@dkuntze
Copy link
Author

dkuntze commented Jan 13, 2025

Ok with the change but aem.js is coming from https://github.com/adobe/aem-lib which brings along the sampleRUM function from https://github.com/adobe/helix-rum-js. This is where the change needs to happen!

I can change it over there as well, but again, I don't have push perms on that repo.

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 wrong, but @kptdobe's suggestion is also wrong, as aem.js gets the update pushed from helix-rum-js. We need to figure out if we can set sampleRUM.collectBaseURL from the outside, and if not add a new variable like window.RUM_COLLECT in helix-rum-js and set it here in the boilerplate.

@dkuntze
Copy link
Author

dkuntze commented Jan 13, 2025

That's wrong, but @kptdobe's suggestion is also wrong, as aem.js gets the update pushed from helix-rum-js. We need to figure out if we can set sampleRUM.collectBaseURL from the outside, and if not add a new variable like window.RUM_COLLECT in helix-rum-js and set it here in the boilerplate.

I'm not sure how these are pushed/merged as they are not identical right now (sampleRUM). Relatively minor differences, but they are not identical.

@trieloff
Copy link
Contributor

aem-lib does not allow auto-updates of helix-rum-js https://github.com/adobe/aem-lib/blob/main/.renovaterc.json we should change that.

@trieloff
Copy link
Contributor

see adobe/aem-lib#124

@dkuntze dkuntze closed this Jan 16, 2025
@dkuntze dkuntze deleted the rummy branch January 16, 2025 22:18
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.

3 participants