-
Notifications
You must be signed in to change notification settings - Fork 18
WEB-3536: fetch api changes #368
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
base: develop
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
src/clevertap.js
Outdated
init (accountId, region, targetDomain, token, antiFlicker = {}) { | ||
if (Object.keys(antiFlicker).length > 0) { | ||
addAntiFlicker(antiFlicker) | ||
init (accountId, region, targetDomain, token, config = { antiFlicker: {} }) { |
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.
This is a breaking change, please add a new param config and do not change the existing function param.
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.
Not being used by anyone currently, confirmed it with @ThisIsRaghavGupta
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.
Before removing it please get a go ahead from the PM. In case we have evidence of no usage we can get rid of it otherwise we have to introduce new argument.
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.
yeah already confirmed this
src/util/requestDispatcher.js
Outdated
document.getElementsByTagName('head')[0].appendChild(s) | ||
this.logger.debug('req snt -> url: ' + url) | ||
} else { | ||
try { |
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.
Create a function handle Fetch response here and use it
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.
done
src/util/requestDispatcher.js
Outdated
} | ||
this.logger.debug('req snt -> url: ' + url) | ||
} catch (error) { | ||
console.error('Fetch error:', error) |
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.
Use logger.error
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.
done
src/util/requestDispatcher.js
Outdated
throw new Error(`Network response was not ok: ${response.statusText}`) | ||
} | ||
const jsonResponse = await response.json() | ||
console.log('Response received:', jsonResponse) |
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.
Use logger.debug
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.
removed this
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
Changes
WEB-3536: Added enableFetchApi flag to conditionally use fetch instead of script injection for firing requests. This flag can be controlled by user while initialising SDK. When flag is missing - default behaviour is script injection for backward compatibility
Changes to Public Facing API if any
Please list the impact on the public facing API if any
How Has This Been Tested?
Cases -
Describe the testing approach and any relevant configurations (e.g., environment, platform)
Overriding SDK
Checklist