-
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
✨ [RUMF-626] use site configuration and deprecate suffixed bundle #476
Conversation
Codecov Report
@@ Coverage Diff @@
## master #476 +/- ##
==========================================
+ Coverage 87.57% 87.73% +0.16%
==========================================
Files 34 34
Lines 2116 2112 -4
Branches 416 413 -3
==========================================
Hits 1853 1853
+ Misses 263 259 -4
Continue to review full report at Codecov.
|
e69dc60
to
6744cb0
Compare
6744cb0
to
e85b1f1
Compare
consistent with other dd libraries remove target env notion, staging file will point to prod by default, site configuration will be used to send data to the right intake
const suffixRegExp = /-(us|eu)/ | ||
const env = fileName.match(suffixRegExp)[1] | ||
const newFileName = fileName.replace(suffixRegExp, '') | ||
return `/**\n * ${fileName} IS DEPRECATED, USE ${newFileName} WITH { site: 'datadoghq.${ |
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.
I fear that a code comment is not enough. What about having a console.warn instead?
Also, https://webpack.js.org/plugins/banner-plugin/ could be used for this purpose.
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.
IMO, adding extra warning is a bit too much, I would do that if we set a date for the removal of the API.
about banner-plugin, since it is not something that we want to maintain, I don't think it worth the effort to change the implementation, wdyt?
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.
Since we don't know how much time we'll need to maintain this, I'd be in favor of having a nicer implementation
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.
I'd say that if we need to touch it, let's push to remove it instead
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.
Alright
Motivation
Allow to easily support future datacenters and align configuration parameter with other libraries.
Changes
datadog-rum.js
anddatadog-logs.js
Testing
Automated tests
I have gone over the contributing documentation.