Handle some quirks mode issues #123

jbuckner · 2013-07-19T17:21:11Z

IE8 and 9 do not support array-style string parsing in quirks mode: "abcdef"[0] returns "" whereas "abcdef".substr(0,1) returns "a"
document.fireEvent() seems to return Invalid argument for almost every argument passed to it in IE8 and IE9 in quirks mode so I wrapped it in a try/catch block to be safe

IE8 and 9 do not support array-style string parsing in quirks mode: "abcdef"[0] returns "" whereas "abcdef".substr(0,1) returns "a" document.fireEvent() returns "Invalid argument" regardless of the parameters sent to it. This happens in IE8 and IE9 in quirks mode.

mattrobenolt · 2013-07-19T17:22:52Z

Is it possible to write a test for this?

jbuckner · 2013-07-19T17:39:10Z

I'm not too familiar with how the tests work, but let me see what I can do. This was done with trial and error in IE8/9 in quirks mode. Is it possible to run tests in IE quirks mode?

mattrobenolt · 2013-07-19T17:59:54Z

I don't really write JavaScript, so I don't know how IE even works. I'd imagine it's possible, but I'm not sure.

Either way, the code looks sane enough and safe. So it's fine. I was more or less trying to understand the problem a bit more.

mattrobenolt · 2013-07-19T18:02:56Z

src/raven.js

@@ -235,7 +235,7 @@ var Raven = {
 function triggerEvent(eventType, options) {
    var event, key;

-    eventType = 'raven' + eventType[0].toUpperCase() + eventType.substr(1);
+    eventType = 'raven' + eventType.substr(0,1).toUpperCase() + eventType.substr(1);


fwiw, substring(0, 1) is generally a tiny bit faster.

a jsperf comparing the two: http://jsperf.com/slice-vs-substr-vs-substring-methods-long-string/14

Handle some quirks mode issues

Add passing extra options to HTTP transport

- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122) - feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126) - feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123) - feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113) - fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121) - ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125) - ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124)

- feat: Export getCanvasManager & allow passing it to record() [#122](getsentry/rrweb#122) - feat: Remove hooks related code, which is not used [#126](getsentry/rrweb#126) - feat: Remove plugins related code, which is not used [#123](getsentry/rrweb#123) - feat: Refactor module scope vars & export mirror & `takeFullSnapshot` directly [#113](getsentry/rrweb#113) - fix(rrweb): Fix rule.style being undefined [#121](getsentry/rrweb#121) - ref: Avoid unnecessary cloning of objects or arrays [#125](getsentry/rrweb#125) - ref: Avoid cloning events to add timestamp [#124](getsentry/rrweb#124) Note: With this update, canvas is _always_ excluded, unless we opt in by passing a `getCanvasManager` function to `record()`. We'll provide a way to do this once we have a fully formed canvas story. For now, this will reduce bundle size considerably for all SDK users.

> * Revert "fix: isCheckout is not included in fullsnapshot event (rrweb-io#1141)" Fixes an issue where Meta event is being lost in buffered replays. > revert: feat: Remove plugins related code, which is not used #123 > feat: Export additional canvas-related types and functions (#134) Changes needed for canvas playback > feat: Skip addHoverClass when stylesheet is >= 1MB #130 Affects playback only

We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevent parts are: ```js <Route path="/" element={<div>root</div>}> <Route path="/issues/:groupId/" element={<div>issues group</div>}> <Route index element={<div>index</div>} /> </Route> </Route> ``` If you navigate to e.g. `/issues/123`, the recorded transaction name is `//issues/:groupId/` (notice the double slash). This is messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (note the trailing slash), the transaction name is `/issues/123/` - it is not parameterized. This breaks transaction grouping. On the `master` and `develop` branch of the SDK, the transaction name is recorded as `/`. This causes the transactions to be grouped incorrectly with the root, as well as other routes with this structure (nested under a route with path `/`). This commit fixes both of these issues and passes both the new tests (which reproduce the above issues) and all existing tests, but I still don't trust the change. First, `newPath` now always has a leading slash, and I don't know how that interacts with the other path concatenations inside `getNormalizedName`. It feels like dealing with path concatenation should be handled in a more systematic way. Second, I revert a change from 3473447 where ```js if (basename + branch.pathname === location.pathname) { ``` became ``` if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that difference in results between SDK versions. This will always match when `basename` is empty, `branch.pathname` is `/`, and `location.pathname` ends with `/` - in other words, if you have a parent route with `path="/"`, every request ending in a slash will match it instead of the more specific child route. This seems likely to be a wide regression in transaction names. But, no tests failed from this change, which makes me worried that there's some necessary behaviour that isn't covered.

@JoshFerge

We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevant parts are: ```js <Route path="/" element={<div>root</div>}> <Route path="/issues/:groupId/" element={<div>issues group</div>}> <Route index element={<div>index</div>} /> </Route> </Route> ``` If you navigate to e.g. `/issues/123` (no trailing slash), the recorded transaction name is `//issues/:groupId/` (notice the double slash). This looks messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (with trailing slash), the transaction name is `/issues/123/` - it is not parameterized. This breaks transaction grouping. On the `master` and `develop` branch of the SDK, the transaction name is recorded as `/`. This causes the transactions to be grouped incorrectly with the root, as well as any other routes nested under a route with `path="/"`. (Thanks @JoshFerge for noticing the bad data in Sentry! 🙌) --- Note that this commit effectively reverts a change from #14304 where ```js if (basename + branch.pathname === location.pathname) { ``` became ```js if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that caused the difference in results between SDK versions. This will always match when `basename` is empty, `branch.pathname` is `/`, and `location.pathname` ends with `/` - in other words, if you have a parent route with `path="/"`, every request ending in a slash will match it instead of the more specific child route. (Depending on how often this kind of `Route` nesting happens in the wild, this could be a wide regression in transaction names.) But, no tests failed from the removal of `endsWith`. @onurtemizkan, who wrote the change in question: > I'd expect this to break descendant routes. But in the final > implementation, descendant routes don't end up here. So, I > believe it's safe to revert.

@JoshFerge

We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevant parts are: ```js <Route path="/" element={<div>root</div>}> <Route path="/issues/:groupId/" element={<div>issues group</div>}> <Route index element={<div>index</div>} /> </Route> </Route> ``` If you navigate to e.g. `/issues/123` (no trailing slash), the recorded transaction name is `//issues/:groupId/` (notice the double slash). This looks messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (with trailing slash), the transaction name is `/issues/123/` - it is not parameterized. This breaks transaction grouping. On the `master` and `develop` branch of the SDK, the transaction name is recorded as `/`. This causes the transactions to be grouped incorrectly with the root, as well as any other routes nested under a route with `path="/"`. (Thanks @JoshFerge for noticing the bad data in Sentry! 🙌) --- Note that this commit effectively reverts a change from #14304 where ```js if (basename + branch.pathname === location.pathname) { ``` became ```js if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that caused the difference in results between SDK versions. This will always match when `basename` is empty, `branch.pathname` is `/`, and `location.pathname` ends with `/` - in other words, if you have a parent route with `path="/"`, every request ending in a slash will match it instead of the more specific child route. (Depending on how often this kind of `Route` nesting happens in the wild, this could be a wide regression in transaction names.) But, no tests failed from the removal of `endsWith`. @onurtemizkan, who wrote the change in question: > I'd expect this to break descendant routes. But in the final > implementation, descendant routes don't end up here. So, I > believe it's safe to revert.

@JoshFerge

We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevant parts are: ```js <Route path="/" element={<div>root</div>}> <Route path="/issues/:groupId/" element={<div>issues group</div>}> <Route index element={<div>index</div>} /> </Route> </Route> ``` If you navigate to e.g. `/issues/123` (no trailing slash), the recorded transaction name is `//issues/:groupId/` (notice the double slash). This looks messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (with trailing slash), the transaction name is `/issues/123/` - it is not parameterized. This breaks transaction grouping. On the `master` and `develop` branch of the SDK, the transaction name is recorded as `/`. This causes the transactions to be grouped incorrectly with the root, as well as any other routes nested under a route with `path="/"`. (Thanks @JoshFerge for noticing the bad data in Sentry! 🙌) --- Note that this commit effectively reverts a change from #14304 where ```js if (basename + branch.pathname === location.pathname) { ``` became ```js if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that caused the difference in results between SDK versions. This will always match when `basename` is empty, `branch.pathname` is `/`, and `location.pathname` ends with `/` - in other words, if you have a parent route with `path="/"`, every request ending in a slash will match it instead of the more specific child route. (Depending on how often this kind of `Route` nesting happens in the wild, this could be a wide regression in transaction names.) But, no tests failed from the removal of `endsWith`. @onurtemizkan, who wrote the change in question: > I'd expect this to break descendant routes. But in the final > implementation, descendant routes don't end up here. So, I > believe it's safe to revert.

mattrobenolt reviewed Jul 19, 2013
View reviewed changes

change substr() to substring() for performance

fbb1bdd

a jsperf comparing the two: http://jsperf.com/slice-vs-substr-vs-substring-methods-long-string/14

mattrobenolt added a commit that referenced this pull request Aug 24, 2013

Merge pull request #123 from yola/quirks_mode_fixes

67ad604

Handle some quirks mode issues

mattrobenolt merged commit 67ad604 into getsentry:master Aug 24, 2013

kahnvex deleted the quirks_mode_fixes branch November 5, 2014 22:04

kamilogorek pushed a commit that referenced this pull request Jun 12, 2018

Merge pull request #123 from maxvipon/pass-extra-options-to-transport

43ff5da

Add passing extra options to HTTP transport

billyvg mentioned this pull request Dec 12, 2023

feat(replay): Bump rrweb to 2.5.0 #9803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle some quirks mode issues #123

Handle some quirks mode issues #123

jbuckner commented Jul 19, 2013

mattrobenolt commented Jul 19, 2013

jbuckner commented Jul 19, 2013

mattrobenolt commented Jul 19, 2013

mattrobenolt Jul 19, 2013

Handle some quirks mode issues #123

Handle some quirks mode issues #123

Conversation

jbuckner commented Jul 19, 2013

mattrobenolt commented Jul 19, 2013

jbuckner commented Jul 19, 2013

mattrobenolt commented Jul 19, 2013

mattrobenolt Jul 19, 2013

Choose a reason for hiding this comment