-
Notifications
You must be signed in to change notification settings - Fork 211
Fixed tested and fixed html examples with no build system #1261
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
Conversation
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.
Pull Request Overview
This PR fixes broken HTML examples in the Rollbar SDK by updating file references and fixing JavaScript issues to ensure examples work properly with a local server.
- Removed references to the deprecated
nojson
distribution variant - Fixed undefined
data
property access in payload configuration - Updated file paths to use relative URLs for better local testing
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
examples/test.html | Updated rollbarJsUrl to use standard UMD build instead of deprecated nojson variant |
examples/snippet.html | Changed hardcoded localhost URL to relative path for better portability |
examples/requirejs/test.html | Updated paths to use minified UMD build instead of deprecated variant |
examples/requirejs/README.md | Updated documentation to reference correct file names |
examples/include_custom_object.html | Added null check to prevent undefined data property access |
bower.json | Removed nojson files from ignore list since they no longer exist |
examples/include_custom_object.html
Outdated
@@ -19,6 +19,7 @@ | |||
// as custom data in the payload. | |||
|
|||
// ensure payload.data.custom is set | |||
payload.data = payload.data || {}; |
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.
If this was necessary, it sounds like a bug. In the SDK, item.data
is the payload. The data key should already be there if this is an item instance, or custom should be directly on payload if it's item.data
.
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 like the first argument is the payload (item.data
), so the intended code here should be:
payload.custom = {myvariable: window.myvariable}
or equivalent.
payload.data.custom
will cause the custom data to not be recognized.
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've tested this proposed change and verified it works as intended (i.e. custom data appears in the Rollbar UI).
--- a/examples/include_custom_object.html
+++ b/examples/include_custom_object.html
@@ -18,12 +18,11 @@
// as an example, let's include the current value of window.myvariable
// as custom data in the payload.
- // ensure payload.data.custom is set
- payload.data = payload.data || {};
- payload.data.custom = payload.data.custom || {};
+ // ensure payload.custom is set
+ payload.custom = payload.custom || {};
// add our data
- payload.data.custom.myvariable = window.myvariable;
+ payload.custom.myvariable = window.myvariable;
// no need to return anything (return value is unused)
};
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.
Thank you for looking into this! Testing all this stuff has been brain numbing!
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.
@waltjones @brianr There does seem to be mentions of payload.data
including payload.data.custom
in tests:
$ rg "payload\.data" -g '!dist/**'
src/truncation.js
18: var body = payload.data.body;
74: var body = payload.data.body;
examples/test.html
8: var filename = payload.data.body.trace.frames[0].filename;
examples/no-conflict/test.html
8: var filename = payload.data.body.trace.frames[0].filename;
examples/universal-browser/test.html
8: var filename = payload.data.body.trace.frames[0].filename;
test/browser.transforms.test.js
546: expect(payload.data.custom.scooby).to.eql('doo');
547: expect(payload.data.custom.okay).to.eql('fizz=buzz&fuzz=baz');
548: expect(payload.data.custom.user.id).to.eql(42);
563: expect(payload.data.custom.okay).to.not.eql('fizz=buzz&fuzz=baz');
564: expect(payload.data.custom.okay).to.match(/fizz=\*+&fuzz=baz/);
565: expect(payload.data.custom.user.id).to.not.be.ok();
566: expect(payload.data.custom.user).to.match(/\*+/);
test/transforms.test.js
155: expect(payload.data.message).to.not.eql('HELLO');
173: expect(payload.data.message).to.not.eql('HELLO');
193: expect(payload.data.message).to.not.eql('HELLO');
219: expect(payload.data.message).to.not.eql('HELLO');
240: expect(payload.data.message).to.not.eql('HELLO');
263: expect(payload.data.message).to.not.eql('HELLO');
test/apiUtility.test.js
14: expect(payload.data).to.eql(data);
22: expect(payload.data.context).to.not.eql(context);
23: expect(payload.data.context).to.eql('{"a":1,"b":"other"}');
34: expect(payload.data.context).to.not.eql(context);
35: expect(payload.data.context.length).to.eql(255);
Are we 100% sure custom
shouldn't be inside payload.data
? Or is this something else?
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 still made the change @ fef9dc4, also tested and it seems to be working fine like this.
I'd still like to know your opinion on payload.data.*
and that that particular payload.data.custom
test in test/browser.transforms.test.js
(maybe the test is broken?)
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.
Those tests are correct, in those payload
actually is the payload which has the data
key, e.g. https://github.com/rollbar/rollbar.js/blob/master/test/browser.transforms.test.js#L541-L546
In the transform functions, the object passed as the first parameter is actually the payload.data
, e.g. https://github.com/rollbar/rollbar.js/blob/master/src/transforms.js#L60
Maybe the docs should name that parameter payloadData
, or some other name besides payload
(could be a separate improvement from this PR).
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, I'll leave the comment unresolved so it stays visible. We can keep talking about this after the PR is merged, or in slack.
Description of the change
This PR fixes some non-working HTML examples and streamlines others so they're easier to test by simply creating a local server (eg.
python3 -m http.server 8000
).nojson
dist variant which doesn't exist anymore.data
wouldn't be defined forpayload
for some reason.Type of change
Related issues
SDK-518/make-sure-all-sdk-examples-work-properly