-
Notifications
You must be signed in to change notification settings - Fork 142
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 to redirect v6-canary bundles urls to local dev bundles #3021
Conversation
@@ -62,11 +62,11 @@ function buildRules( | |||
const devRumUrl = useRumSlim ? DEV_RUM_SLIM_URL : DEV_RUM_URL | |||
logger.log('add redirect to dev bundles rules') | |||
rules.push( | |||
createRedirectRule(/^https:\/\/.*\/datadog-rum(-v\d|-canary|-staging)?\.js$/, { url: devRumUrl }), | |||
createRedirectRule(/^https:\/\/.*\/datadog-rum-slim(-v\d|-canary|-staging)?\.js$/, { | |||
createRedirectRule(/^https:\/\/.*\/datadog-rum(-v\d|-canary|-staging|-v\d-canary)?\.js$/, { url: devRumUrl }), |
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.
💬 suggestion: I think we should simplify the regex to make it future proof: datatdog-rum(-\w+)?\.js
. It's pretty low-risk.
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.
💭 Though I think there is an upside of having the explicit rules: it can serve as docs for the versions we have and always updated.
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.
We have better documentation here though (although a bit outdated): https://datadoghq.atlassian.net/wiki/spaces/RUMP/pages/2523531139/SDK+distribution#CDN
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.
fixed, Mainly for simplification as I'm not totally convinced that it's futureproof, especially with the split bundles initiative, but we will fix it in due time.
I'll update the doc too, thanks
@@ -62,11 +62,11 @@ function buildRules( | |||
const devRumUrl = useRumSlim ? DEV_RUM_SLIM_URL : DEV_RUM_URL | |||
logger.log('add redirect to dev bundles rules') | |||
rules.push( | |||
createRedirectRule(/^https:\/\/.*\/datadog-rum(-v\d|-canary|-staging)?\.js$/, { url: devRumUrl }), | |||
createRedirectRule(/^https:\/\/.*\/datadog-rum-slim(-v\d|-canary|-staging)?\.js$/, { | |||
createRedirectRule(/^https:\/\/.*\/datadog-rum(-v\d|-canary|-staging|-v\d-canary)?\.js$/, { url: devRumUrl }), |
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.
💭 Though I think there is an upside of having the explicit rules: it can serve as docs for the versions we have and always updated.
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
Motivation
Enable local dev bundle to be loaded properly even when the
browser_sdk_next_major_version
is enabledChanges
Tweak the redirect rules to account for the
v6-canary
bundles loaded when the next major feature flags is enabledTesting
I have gone over the contributing documentation.