feat(node): add versioned defaults and strictCapture option#3261
Open
feat(node): add versioned defaults and strictCapture option#3261
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Contributor
|
Size Change: +1.18 kB (+0.02%) Total Size: 6.57 MB
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/node/src/__tests__/posthog-node.spec.ts
Line: 416-484
Comment:
**Prefer parameterised tests over repetition**
The new tests follow a nearly identical pattern across `capture` / `captureImmediate` and the two activation paths (`strictCapture: true` vs `defaults: '2026-03-19'`). Per the project's simplicity rules (OnceAndOnlyOnce) and preference for parameterised tests, these could be collapsed significantly. For example, the four throw-case tests could become:
```typescript
it.each([
['capture', 'strictCapture: true', { strictCapture: true }],
['capture', 'defaults: 2026-03-19', { defaults: '2026-03-19' }],
['captureImmediate', 'strictCapture: true', { strictCapture: true }],
['captureImmediate', 'defaults: 2026-03-19', { defaults: '2026-03-19' }],
])('should throw if %s is called with a string when %s', async (method, _label, extraOptions) => {
const ph = new PostHog('TEST_API_KEY', { host: 'http://example.com', ...extraOptions })
// @ts-expect-error - Testing the error when passing a string instead of an object
await expect(Promise.resolve().then(() => ph[method]('test-event'))).rejects.toThrow(TypeError)
await ph.shutdown()
})
```
Similarly the two warn-path tests could be unified into a single `it.each` block.
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/node/src/__tests__/posthog-node.spec.ts
Line: 456-484
Comment:
**Warning path for `captureImmediate` is not tested**
The "should warn" tests only exercise `ph.capture(...)`. The symmetric branch in `captureImmediate` (lines 511-516 of `client.ts`) also emits the same warning when `strictCapture` is false, but there is no corresponding test for it. Given the `capture` and `captureImmediate` paths are separate code blocks, adding coverage for `captureImmediate` here (or via parameterisation as suggested above) would close that gap.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "feat(node): add vers..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+456
to
+484
| it('should warn if capture is called with a string when defaults is unset', () => { | ||
| const ph = new PostHog('TEST_API_KEY', { | ||
| host: 'http://example.com', | ||
| defaults: 'unset', | ||
| }) | ||
| ph.debug(true) | ||
| // @ts-expect-error - Testing the warning when passing a string instead of an object | ||
| ph.capture('test-event') | ||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| '[PostHog]', | ||
| 'Called capture() with a string as the first argument when an object was expected.' | ||
| ) | ||
| ph.shutdown() | ||
| }) | ||
|
|
||
| it('should allow explicit strictCapture: false to override defaults', async () => { | ||
| const ph = new PostHog('TEST_API_KEY', { | ||
| host: 'http://example.com', | ||
| defaults: '2026-03-19', | ||
| strictCapture: false, | ||
| }) | ||
| ph.debug(true) | ||
| // @ts-expect-error - Testing the warning when passing a string instead of an object | ||
| ph.capture('test-event') | ||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| '[PostHog]', | ||
| 'Called capture() with a string as the first argument when an object was expected.' | ||
| ) | ||
| await ph.shutdown() |
Contributor
There was a problem hiding this comment.
captureImmediate is not tested
The "should warn" tests only exercise ph.capture(...). The symmetric branch in captureImmediate (lines 511-516 of client.ts) also emits the same warning when strictCapture is false, but there is no corresponding test for it. Given the capture and captureImmediate paths are separate code blocks, adding coverage for captureImmediate here (or via parameterisation as suggested above) would close that gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/node/src/__tests__/posthog-node.spec.ts
Line: 456-484
Comment:
**Warning path for `captureImmediate` is not tested**
The "should warn" tests only exercise `ph.capture(...)`. The symmetric branch in `captureImmediate` (lines 511-516 of `client.ts`) also emits the same warning when `strictCapture` is false, but there is no corresponding test for it. Given the `capture` and `captureImmediate` paths are separate code blocks, adding coverage for `captureImmediate` here (or via parameterisation as suggested above) would close that gap.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0df66e7581
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
marandaneto
reviewed
Mar 20, 2026
| this._logger.warn('Called capture() with a string as the first argument when an object was expected.') | ||
| const msg = 'Called capture() with a string as the first argument when an object was expected.' | ||
| if (this.options.strictCapture) { | ||
| throw new TypeError(msg) |
Member
There was a problem hiding this comment.
IMO SDKs should never throw, SDKs should degrade gracefully and warn/log out if something is wrong (which is whats happening here).
Curious why people called with the wrong params tho, though. Was it not up-to-date documentation? LLMs guessing public APIs?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
We have seen some cases of people using the posthog-js method signature for capturing events like
capture(event: string, properties?: PostHogEventProperties, options?: PostHogCaptureOptions): voidinstead ofcapture(props: EventMessage): voidthis would bring up a warning logCalled capture() with a string as the first argument when an object was expected.but many undesired behavior as well.Changes
Add a new
defaultsoption to avoid a breaking change and a newstrictCaptureconfig that when combined with the new default config option will make the code path throw instead of just warning.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file