-
Notifications
You must be signed in to change notification settings - Fork 53
[RUM-11388] Fix FileBasedConfiguration related issues #964
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
[RUM-11388] Fix FileBasedConfiguration related issues #964
Conversation
buraizu
left a comment
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.
Just flagging some typos and suggesting a few other updates for consistency
@buraizu Hi, thanks for raising all those typos 🙇 If you don't mind, I'll open a separate PR with your changes, as this specific PR is aimed toward a feature branch, and I think it's better to apply these corrections directly on develop. EDIT: I've opened a PR with these fixes here. |
1a1da93 to
b1ae8ee
Compare
b1ae8ee to
6b944a9
Compare
6b944a9 to
89f08ac
Compare
| // This corresponds to a file located at the root of a RN project. | ||
| // /!\ We have to write the require this way as dynamic requires are not supported by Hermes. | ||
| // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires | ||
| const jsonContent = require('../../../../../../datadog-configuration.json'); |
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.
So now we will no longer try to resolve the configuration file ourselves automatically ? We'll require it to be passed so we don't have to rely on that flaky path correct ?
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.
Exactly, passing a specific path to the configuration file will be mandatory on v3 and we won't try to guess it on our own as it was prone to errors and ended up being flimsy.
What does this PR do?
This PR aims to fix several issues with FileBasedConfiguration initialization of the SDK.
1.- Fixes a reported issue that makes it so directly passing a configuration json file enforces users to duplicate and nest another
configurationproperty for it to work, ignoring the structure defined ondatadog-configuration.schema.json. Doing so makes the schema validator label the json configuration as invalid. I fixed it so it properly accepts the schema we enforce with just oneconfigurationentry.This means that creating a new FileBasedConfiguration can be done like so:
2.- Removes the automatic loading of a datadog-configuration.json file by not passing a config file to FileBasedConfiguration(). The way in which this was done was flimsy, as it relied on jumping out of node_modules up to the root app folder. This worked most of the time but it caused an issue generating a require cycle that made 2 of our internal enums to be undefined at runtime (TrackingConsent and SdkVerbosity).
3.- Updates the FileBasedConfiguration tests to be more thorough and cover edge cases properly.
4.- Expands the example app showcasing different ways to provide a configuration object to the DatadogProvider, including FileBasedConfiguration schemes.
Motivation
FileBasedConfiguration should consistently work across the board.
Additional notes
This is a breaking change, as the option of automatically looking for a config.json file without having the user provide a specific path when creating a
FileBasedConfigurationobject will cease to work.Review checklist (to be filled by reviewers)