fix(core): keep safeJoin side-effect-free for DOM elements (#21353)#21356
Conversation
…#21353) In 10.55.0 safeJoin switched non-primitive serialization from String() to stringifyValue(), which for DOM elements routes through the browser stringifier's htmlTreeAsString and reads id/className/getAttribute, invoking user-defined getters (and walking the DOM) during console breadcrumb capture. Short-circuit elements via String() so breadcrumb serialization stays side-effect free. Adds a regression test.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This clearly fixes the regression described, and restores the behavior very surgically without affecting the intended behavior that led to it. Good patch, thanks!
andreiborza
requested review from
logaretm and
mydea
and removed request for
a team
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
While I agree that it restores behavior and avoids invoking user defined getters, I think we can get to a state where it does not invoke getters but supports the nicer tree rendering.
I changed the behavior a bit.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
andreiborza
requested review from
chargome and
nicohrubec
and removed request for
logaretm and
mydea
| } | ||
|
|
||
| function _safeRead<T>(el: unknown, prop: AccessorKey, arg?: string): T { | ||
| return accessors[prop]!.call(el, arg) as T; |
There was a problem hiding this comment.
q: Is this a safe read actually? Should we trycatch these?
There was a problem hiding this comment.
Good point, added try/catch with fallback to just using the field in 04f75df
|
|
||
| if (typeof Node !== 'undefined') { | ||
| // oxlint-disable-next-line typescript-eslint(unbound-method) | ||
| accessors.parentNode = Object.getOwnPropertyDescriptor(Node.prototype, 'parentNode')!.get!; |
There was a problem hiding this comment.
Maybe we just defensively put these into trycatch to guard against breaking in case this got stubbed, polyfilled or whatever
There was a problem hiding this comment.
Added, wrapped over all of them. If one of them fails we just fall back to direct access in 04f75df
andreiborza
requested review from
Lms24,
andreiborza,
chargome and
logaretm
and removed request for
nicohrubec
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #21356 Co-authored-by: isaacs <9287+isaacs@users.noreply.github.com> Co-authored-by: Nicolas Hrubec <nicolas.hrubec@outlook.com>
4 participants
Closes #21353
oxlint) and the affected test suite passes (vitest run packages/core/test/lib/utils/string.test.ts→ 29/29).Summary
In
10.55.0,safeJoin(packages/core/src/utils/string.ts) switched non-primitive serialization fromString(value)tostringifyValue(...). For DOM elements,stringifyValueroutes through the browser stringifier'shtmlTreeAsString, which readsid/className/getAttributeand walks the ancestor chain — so capturing a console breadcrumb now invokes user-defined property getters and traverses the DOM on everyconsole.log(element), even when no event is sent. (<= 10.54.0was side-effect-free viaString(value).)This short-circuits DOM elements in
safeJoinback to a side-effect-freeString(value), while keepingstringifyValuefor other non-primitive values (functions, plain objects, etc.).htmlTreeAsStringis intentionally left intact — it is still used for the click/DOM breadcrumb path that legitimately needs the selector string.Added a regression test asserting
safeJoindoes not invoke getters or the stringifier for elements (it fails on the current code and passes with this change).