fix(core): use typeof check in isEvent to prevent ReferenceError in non-browser runtimes#3269
fix(core): use typeof check in isEvent to prevent ReferenceError in non-browser runtimes#3269leifarriens wants to merge 1 commit intoPostHog:mainfrom
Conversation
…on-browser runtimes
|
@leifarriens is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/core/src/utils/type-utils.spec.ts
Line: 34-38
Comment:
**Test can silently pass without asserting**
The `if (typeof Event !== 'undefined')` guard means this test will pass vacuously in any environment where `Event` is not defined (e.g., a Node.js test runner without jsdom). No assertion is ever executed and the test reports success while actually verifying nothing.
Consider using `it.skipIf` (Vitest) or a similar mechanism to make the skip explicit and visible:
```
it.skipIf(typeof Event === 'undefined')('returns true for Event instances when Event global exists', () => {
expect(isEvent(new Event('test'))).toBe(true)
})
```
Or, if the test environment is always jsdom (where `Event` is defined), remove the guard entirely since it's never needed.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/core/src/utils/type-utils.spec.ts
Line: 40-46
Comment:
**Prefer parameterised test**
The existing tests in this file use `it.each` for parameterised cases (see `isNumber` and `isPositiveNumber`). This test checks multiple non-Event values individually — it should follow the same pattern for consistency:
```suggestion
it.each([
[new Error('test'), false],
['string', false],
[{}, false],
[null, false],
[undefined, false],
])('isEvent(%p) returns %p', (value, expected) => {
expect(isEvent(value)).toBe(expected)
})
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix(core): use typeo..." |
| it('returns true for Event instances when Event global exists', () => { | ||
| if (typeof Event !== 'undefined') { | ||
| expect(isEvent(new Event('test'))).toBe(true) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Test can silently pass without asserting
The if (typeof Event !== 'undefined') guard means this test will pass vacuously in any environment where Event is not defined (e.g., a Node.js test runner without jsdom). No assertion is ever executed and the test reports success while actually verifying nothing.
Consider using it.skipIf (Vitest) or a similar mechanism to make the skip explicit and visible:
it.skipIf(typeof Event === 'undefined')('returns true for Event instances when Event global exists', () => {
expect(isEvent(new Event('test'))).toBe(true)
})
Or, if the test environment is always jsdom (where Event is defined), remove the guard entirely since it's never needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/utils/type-utils.spec.ts
Line: 34-38
Comment:
**Test can silently pass without asserting**
The `if (typeof Event !== 'undefined')` guard means this test will pass vacuously in any environment where `Event` is not defined (e.g., a Node.js test runner without jsdom). No assertion is ever executed and the test reports success while actually verifying nothing.
Consider using `it.skipIf` (Vitest) or a similar mechanism to make the skip explicit and visible:
```
it.skipIf(typeof Event === 'undefined')('returns true for Event instances when Event global exists', () => {
expect(isEvent(new Event('test'))).toBe(true)
})
```
Or, if the test environment is always jsdom (where `Event` is defined), remove the guard entirely since it's never needed.
How can I resolve this? If you propose a fix, please make it concise.| it('returns false for non-Event values', () => { | ||
| expect(isEvent(new Error('test'))).toBe(false) | ||
| expect(isEvent('string')).toBe(false) | ||
| expect(isEvent({})).toBe(false) | ||
| expect(isEvent(null)).toBe(false) | ||
| expect(isEvent(undefined)).toBe(false) | ||
| }) |
There was a problem hiding this comment.
The existing tests in this file use it.each for parameterised cases (see isNumber and isPositiveNumber). This test checks multiple non-Event values individually — it should follow the same pattern for consistency:
| it('returns false for non-Event values', () => { | |
| expect(isEvent(new Error('test'))).toBe(false) | |
| expect(isEvent('string')).toBe(false) | |
| expect(isEvent({})).toBe(false) | |
| expect(isEvent(null)).toBe(false) | |
| expect(isEvent(undefined)).toBe(false) | |
| }) | |
| it.each([ | |
| [new Error('test'), false], | |
| ['string', false], | |
| [{}, false], | |
| [null, false], | |
| [undefined, false], | |
| ])('isEvent(%p) returns %p', (value, expected) => { | |
| expect(isEvent(value)).toBe(expected) | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/utils/type-utils.spec.ts
Line: 40-46
Comment:
**Prefer parameterised test**
The existing tests in this file use `it.each` for parameterised cases (see `isNumber` and `isPositiveNumber`). This test checks multiple non-Event values individually — it should follow the same pattern for consistency:
```suggestion
it.each([
[new Error('test'), false],
['string', false],
[{}, false],
[null, false],
[undefined, false],
])('isEvent(%p) returns %p', (value, expected) => {
expect(isEvent(value)).toBe(expected)
})
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Problem
Error tracking in
posthog-react-nativeis non-functional. When initializing PostHog with error tracking autocapture enabled:No
$exceptionevents are ever sent. TheisEvent()utility in@posthog/coredirectly references the Event global, which doesn't exist in React Native's Hermes runtime. The resulting error surfaces as:Changes
typeof Event !== 'undefined'instead of!isUndefined(Event)inisEvent()to safely handle runtimes where Event doesn't exist.isEvent().Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file