-
Notifications
You must be signed in to change notification settings - Fork 144
Bug/sdk 2455/fix sdk for turkish servers #720
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
Bug/sdk 2455/fix sdk for turkish servers #720
Conversation
* Check for last instance of .js extension in filename * adding clarifying comments for change * Update scripts/upload-assets.js Co-authored-by: Seth Silesky <[email protected]> Co-authored-by: Seth Silesky <[email protected]>
* Add WalkMe SRI Fix previous test and add new cases * Fix unit test Co-authored-by: Francisco Garcia <[email protected]>
@@ -17,11 +17,8 @@ var CleverTap = (module.exports = integration('CleverTap') | |||
.global('clevertap') | |||
.option('clevertap_account_id', '') | |||
.option('region', '') | |||
.tag('http', '<script src="http://static.clevertap.com/js/a.js">') | |||
.tag( |
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.
There is a logic further down the code (in initialize
method) that does this:
var protocol = useHttps() ? 'https' : 'http';
this.load(protocol, this.ready);
the this.load
method uses protocol
to decide which script to load, but with this change we removed the https
script, and therefore, the this.load
will fail for https
cases.
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.
hi @pooyaj , If i understood this right, then the this.load
will fail for http, since in the tag I've changed it to https
now. The intent is to load the script over https
only, since most of the people use https.
Also I think use-https
library no longer serves the purpose, so would be removing this due to above stated reasons.
Thanks!
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.
Looks great!
What does this PR do?
Are there breaking changes in this PR?
Testing
Any background context you want to provide?
Is there parity with the server-side/android/iOS integration components (if applicable)?
Does this require a new integration setting? If so, please explain how the new setting works
Links to helpful docs and other external resources