-
Notifications
You must be signed in to change notification settings - Fork 120
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
Github Issue #137: Add helper function to inject javascript snippet in views #186
Github Issue #137: Add helper function to inject javascript snippet in views #186
Conversation
Ready for review |
src/RollbarJsHelper.php
Outdated
*/ | ||
public function addJs($headers = null, $nonce = null) | ||
{ | ||
return |
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 this returns one string with two script tags, I think some browsers can execute script tags in arbitrary order so we suggest in the rollbar.js docs putting both the _rollbarConfig variable definition and the snippet inside one script tag. For most browsers it doesn't matter as things are executed top to bottom, but to be safe we should generate one script tag which has both the config var and the snippet.
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.
Ok, will squash this in one script tag. Although, I'm pretty sure that is not how rollbar-gem does this:
https://github.com/rollbar/rollbar-gem/blob/master/lib/rollbar/middleware/js.rb#L85
https://github.com/rollbar/rollbar-gem/blob/master/lib/rollbar/middleware/js.rb#L105-L111
We might fix this in rollbar-gem as well then.
Nice |
Implementation of injecting RollbarJS snippet through PHP SDK similar to the Rollbar gem #137
This is still work in progress.