Skip to content

Commit 2f5ce39

Browse files
HazATHazAT
authored
fix: Loader callback order (#1875)
* fix: Loader callback order * feat: Add test for callback order * feat: Add additional test * feat: Add more tests * fix: Loader, append sdk to body * fix: tests * Update CHANGELOG.md * Apply suggestions from code review Co-Authored-By: HazAT <daniel.griesser.86@gmail.com>
1 parent f6fc304 commit 2f5ce39

File tree

9 files changed

+351
-178
lines changed

9 files changed

+351
-178
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
## Unreleased
44

5+
- [loader] fix: Detect if `init` has been called in an onload callback
6+
- [core] ref: Multiple `init` calls have been changed to "latest wins" instead of "ignore all after first"
57
- [node] feat: If `options.dsn` is undefined when calling `init` we try to load it from `process.env.SENTRY_DSN`
68
- [node] feat: Expose `flush` and `close` on `Sentry.*`
79
- [node] feat: Add `sentry` to express error handler response which contains the `event_id` of the error

‎packages/browser/src/loader.js‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@
5858
// come out in the wrong order. Because of that we don't need async=1 as GA does.
5959
// it was probably(?) a legacy behavior that they left to not modify few years old snippet
6060
// https://www.html5rocks.com/en/tutorials/speed/script-loading/
61-
var _currentScriptTag = _document.getElementsByTagName(_script)[0];
6261
var _newScriptTag = _document.createElement(_script);
6362
_newScriptTag.src = _sdkBundleUrl;
6463
_newScriptTag.crossorigin = 'anonymous';
@@ -91,40 +90,48 @@
9190
}
9291
});
9392

94-
_currentScriptTag.parentNode.insertBefore(_newScriptTag, _currentScriptTag);
93+
// We append the script to the body because if you use an `onload` callback it could happen that
94+
// the `onLoad` of the already injected SDK will be called, which breaks the setup flow.
95+
document.body.appendChild(_newScriptTag);
9596
}
9697

9798
function sdkLoaded(callbacks, SDK) {
9899
try {
100+
var data = queue.data;
101+
102+
// We have to make sure to call all callbacks first
99103
for (var i = 0; i < callbacks.length; i++) {
100104
if (typeof callbacks[i] === 'function') {
101105
callbacks[i]();
102106
}
103107
}
104108

105-
var data = queue.data;
109+
var initAlreadyCalled = false;
110+
// If there is a global __SENTRY__ that means that in any of the callbacks init() was already invoked
111+
if (!(typeof _window['__SENTRY__'] === 'undefined')) {
112+
initAlreadyCalled = true;
113+
}
106114

107-
// We want to replay all calls to Sentry first to make sure init is called before
108-
// we call all our internal error handlers
109-
var firstInitCall = false;
115+
// We want to replay all calls to Sentry and also make sure that `init` is called if it wasn't already
116+
// We replay all calls to `Sentry.*` now
110117
var calledSentry = false;
111118
for (var i = 0; i < data.length; i++) {
112119
if (data[i].f) {
113120
calledSentry = true;
114121
var call = data[i];
115-
if (firstInitCall === false && call.f !== 'init') {
116-
// First call always has to be init, this is a conveniece for the user
117-
// so call to init is optional
122+
if (initAlreadyCalled === false && call.f !== 'init') {
123+
// First call always has to be init, this is a conveniece for the user so call to init is optional
118124
SDK.init();
119125
}
120-
firstInitCall = true;
126+
initAlreadyCalled = true;
121127
SDK[call.f].apply(SDK, call.a);
122128
}
123129
}
124-
if (calledSentry === false) {
130+
if (initAlreadyCalled === false && calledSentry === false) {
125131
// Sentry has never been called but we need Sentry.init() so call it
126132
SDK.init();
127133
}
134+
128135
// Because we installed the SDK, at this point we have an access to TraceKit's handler,
129136
// which can take care of browser differences (eg. missing exception argument in onerror)
130137
var tracekitErrorHandler = _window[_onerror];
@@ -200,7 +207,7 @@
200207
};
201208

202209
if (!lazy) {
203-
setTimeout(function() {
210+
setTimeout(function () {
204211
injectSdk(onLoadCallbacks);
205212
});
206213
}

‎packages/browser/test/integration/common.js‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,78 @@
7878
KeyboardEvent.prototype = Event.prototype;
7979
window.KeyboardEvent = KeyboardEvent;
8080
})();
81+
82+
(function() {
83+
// store references to original, unwrapped built-ins in order to:
84+
// - get a clean, unwrapped setTimeout (so stack traces don't include
85+
// frames from mocha)
86+
// - make assertions re: wrapped functions
87+
window.originalBuiltIns = {
88+
setTimeout: setTimeout,
89+
setInterval: setInterval,
90+
requestAnimationFrame: requestAnimationFrame,
91+
xhrProtoOpen: XMLHttpRequest.prototype.open,
92+
headAddEventListener: document.head.addEventListener, // use <head> 'cause body isn't closed yet
93+
headRemoveEventListener: document.head.removeEventListener,
94+
consoleDebug: console.debug,
95+
consoleInfo: console.info,
96+
consoleWarn: console.warn,
97+
consoleError: console.error,
98+
consoleLog: console.log,
99+
};
100+
101+
// expose events so we can access them in our tests
102+
window.sentryData = [];
103+
window.sentryBreadcrumbs = [];
104+
})();
105+
106+
function initSDK() {
107+
// stub transport so we don't actually transmit any data
108+
function DummyTransport() {}
109+
DummyTransport.prototype.sendEvent = function(event) {
110+
sentryData.push(JSON.parse(event));
111+
done(sentryData);
112+
return Promise.resolve({
113+
status: 'success',
114+
});
115+
};
116+
117+
Sentry.init({
118+
dsn: 'https://public@example.com/1',
119+
// debug: true,
120+
attachStacktrace: true,
121+
transport: DummyTransport,
122+
ignoreErrors: ['ignoreErrorTest'],
123+
blacklistUrls: ['foo.js'],
124+
// integrations: function(old) {
125+
// return [new Sentry.Integrations.Debug({ stringify: true })].concat(old);
126+
// },
127+
beforeBreadcrumb: function(breadcrumb) {
128+
// Filter console logs as we use them for debugging *a lot* and they are not *that* important
129+
// But allow then if we explicitly say so (for one of integration tests)
130+
if (breadcrumb.category === 'console' && !window.allowConsoleBreadcrumbs) {
131+
return null;
132+
}
133+
134+
// overlyComplicatedDebuggingMechanism 'aka' console.log driven debugging
135+
// console.log(JSON.stringify(breadcrumb, null, 2));
136+
137+
// Filter internal Karma requests
138+
if (
139+
breadcrumb.type === 'http' &&
140+
(breadcrumb.data.url.indexOf('test.js') !== -1 || breadcrumb.data.url.indexOf('frame.html') !== -1)
141+
) {
142+
return null;
143+
}
144+
145+
// Filter "refresh" like navigation which occurs in Mocha when testing on Android 4
146+
if (breadcrumb.category === 'navigation' && breadcrumb.data.to === breadcrumb.data.from) {
147+
return null;
148+
}
149+
150+
sentryBreadcrumbs.push(breadcrumb);
151+
152+
return breadcrumb;
153+
},
154+
});
155+
}

‎packages/browser/test/integration/init.js‎

Lines changed: 1 addition & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1,74 +1,4 @@
1-
// store references to original, unwrapped built-ins in order to:
2-
// - get a clean, unwrapped setTimeout (so stack traces don't include
3-
// frames from mocha)
4-
// - make assertions re: wrapped functions
5-
window.originalBuiltIns = {
6-
setTimeout: setTimeout,
7-
setInterval: setInterval,
8-
requestAnimationFrame: requestAnimationFrame,
9-
xhrProtoOpen: XMLHttpRequest.prototype.open,
10-
headAddEventListener: document.head.addEventListener, // use <head> 'cause body isn't closed yet
11-
headRemoveEventListener: document.head.removeEventListener,
12-
consoleDebug: console.debug,
13-
consoleInfo: console.info,
14-
consoleWarn: console.warn,
15-
consoleError: console.error,
16-
consoleLog: console.log,
17-
};
18-
19-
// expose events so we can access them in our tests
20-
window.sentryData = [];
21-
window.sentryBreadcrumbs = [];
22-
23-
// stub transport so we don't actually transmit any data
24-
function DummyTransport() {}
25-
DummyTransport.prototype.sendEvent = function(event) {
26-
// console.log(JSON.stringify(event, null, 2));
27-
sentryData.push(JSON.parse(event));
28-
done(sentryData);
29-
return Promise.resolve({
30-
status: 'success',
31-
});
32-
};
33-
34-
Sentry.init({
35-
dsn: 'https://public@example.com/1',
36-
// debug: true,
37-
attachStacktrace: true,
38-
transport: DummyTransport,
39-
ignoreErrors: ['ignoreErrorTest'],
40-
blacklistUrls: ['foo.js'],
41-
// integrations: function(old) {
42-
// return [new Sentry.Integrations.Debug({ stringify: true })].concat(old);
43-
// },
44-
beforeBreadcrumb: function(breadcrumb) {
45-
// Filter console logs as we use them for debugging *a lot* and they are not *that* important
46-
// But allow then if we explicitly say so (for one of integration tests)
47-
if (breadcrumb.category === 'console' && !window.allowConsoleBreadcrumbs) {
48-
return null;
49-
}
50-
51-
// overlyComplicatedDebuggingMechanism 'aka' console.log driven debugging
52-
// console.log(JSON.stringify(breadcrumb, null, 2));
53-
54-
// Filter internal Karma requests
55-
if (
56-
breadcrumb.type === 'http' &&
57-
(breadcrumb.data.url.indexOf('test.js') !== -1 || breadcrumb.data.url.indexOf('frame.html') !== -1)
58-
) {
59-
return null;
60-
}
61-
62-
// Filter "refresh" like navigation which occurs in Mocha when testing on Android 4
63-
if (breadcrumb.category === 'navigation' && breadcrumb.data.to === breadcrumb.data.from) {
64-
return null;
65-
}
66-
67-
sentryBreadcrumbs.push(breadcrumb);
68-
69-
return breadcrumb;
70-
},
71-
});
1+
initSDK(); // can be found in common.js
722

733
function bar() {
744
baz();

‎packages/browser/test/integration/loader-with-no-global-init-lazy-no.html‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
<title></title>
6+
<script src="polyfills/es6-promise-4.2.5.auto.js"></script>
7+
<script src="polyfills/whatwg-fetch-3.0.0.js"></script>
8+
<script src="common.js"></script>
9+
<script src="../../src/loader.js" data-lazy="no"></script>
10+
</head>
11+
12+
<body></body>
13+
</html>

‎packages/browser/test/integration/loader-with-no-global-init.html‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
<title></title>
6+
<script src="polyfills/es6-promise-4.2.5.auto.js"></script>
7+
<script src="polyfills/whatwg-fetch-3.0.0.js"></script>
8+
<script src="common.js"></script>
9+
<script src="../../src/loader.js"></script>
10+
</head>
11+
12+
<body></body>
13+
</html>

0 commit comments

Comments
 (0)