Skip to content

fix(event): support both positional and object args in parse() - #172

Open
mateussiqueira wants to merge 1 commit into
starkbank:masterfrom
mateussiqueira:master
Open

mateussiqueira wants to merge 1 commit into
starkbank:masterfrom
mateussiqueira:master

Conversation

@mateussiqueira

Copy link
Copy Markdown

Bug

event.parse() only accepted a single object parameter via destructuring:

parse({content, signature, user})

But the JSDoc documents positional parameters:

parse(content, signature, user)

When users follow the docs and call parse(content, signature), destructuring the content string yields undefined for all values, causing JSON.parse(undefined)SyntaxError: Unexpected token u in JSON at position 0.

Fix

  1. Check if first arg is a plain object → destructure; otherwise treat as positional
  2. Add input validation: throw clear error if content/signature are missing

Closes #163

The parse() function only accepted a single object parameter via
destructuring:

  parse({content, signature, user})

But its JSDoc documents positional parameters:

  parse(content, signature, user)

When users follow the docs and call parse(content, signature),
destructuring the content string yields undefined for all values,
causing JSON.parse(undefined) → 'Unexpected token u in JSON at position 0'.

Add a compatibility check at the top of the function: if the first
argument is a plain object, use destructuring; otherwise treat them
as positional. Also add input validation so undefined/null content
or signature throws a clear error instead of a cryptic JSON syntax error.

Fixes starkbank#163

@edu-stark edu-stark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review against the Python reference and the backend services (read-only pass over every open PR, 2026-09-17). Findings below; happy to help with the rebase once the content points are addressed.

#172 — fix(event): support both positional and object args in parse()

  • Repo: /Users/eduardo.santos/git/starkbank/sdk-node (default master, tip 281ea55)
  • Head ref: origin/pr-172 = d1b19a3
  • Author: mateussiqueira (external)
  • Verdict: request-changes

What the change actually is

The branch is 313 commits / 112 merge commits, but exactly one commit is the author's own
work, and it touches one file, 12 insertions / 1 deletion:

  • sdk/event/event.js:171 — signature flipped from async function ({content, signature, user} = {}) to async function (content, signature, user)
  • sdk/event/event.js:192-194 — compat branch: if arg 1 is a non-null non-array object, destructure it
  • sdk/event/event.js:196-201 — two bare throw new Error(...) guards on content / signature

Everything else in the 45-file delta is the fork being behind, not the author changing things.

Blocking findings

1. Buffer regression — breaks the README's own webhook snippet (sdk/event/event.js:196)

JSON.parse(buffer) works today (verified in node: JSON.parse(Buffer.from('{"event":{}}')) parses
fine), so parse({content: req.body, signature}) with an express.raw() Buffer body works on master.
With this PR that call reaches line 196, typeof content !== 'string' is true, and it throws
content must be a non-empty JSON string. Working integrations start failing on upgrade.
Worse in the positional form the PR is meant to enable: parse(req.body, sig) — a Buffer is
typeof 'object', non-null, not an Array, so line 192 destructures the Buffer, yielding
content === undefined (verified), and it throws the same error. The one call shape the PR advertises
is the one that breaks on the most common Express webhook setup.

2. The stated premise is wrong; this is an unapproved public-API change, not a bugfix

The commit message claims "its JSDoc documents positional parameters". It does not — @param content
under Parameters (required): is this repo's uniform docstring shape for object params
(exports.query at sdk/event/event.js:69 documents @param limit the same way and is destructured).
The documented surface is unambiguously the object form:

  • types/event/event.d.ts:105export function parse(params?: {content: string, signature: string, user?: Project | Organization}): Promise<Event>
  • README.md:2802starkbank.event.parse({content: ..., signature: ...})
  • tests/testEvent.js:44,55,71 and testTypes/Event.test.ts:47,58,74 — all object form
  • /Users/eduardo.santos/git/starkinfra/sdk-node/sdk/event/event.js:166 — identical object signature (cross-SDK Node parity)

Python is positional (/Users/eduardo.santos/git/starkbank/sdk-python/starkbank/event/__event.py:154,
def parse(content, signature, user=None)) but that is Python kwargs idiom, not the Node surface.
Making positional canonical in Node changes a public API and breaks parity with starkinfra/sdk-node —
that needs a decision, plus .d.ts, README, CHANGELOG and a matching starkinfra PR, none of which are here.

3. .d.ts not updated (types/event/event.d.ts:105)

TypeScript users cannot call the new positional form at all — parse(content, signature) is a type
error against the current declaration. The PR's whole point is unreachable from TS.

4. Bare Error instead of the repo's error classes (sdk/event/event.js:197,200)

sdk/error.js exports InputError/InputErrors/InvalidSignatureError, all extending
StarkBankError. New validation must throw one of those; a bare Error cannot be caught by
e instanceof starkbank.error.*, which is the pattern the README (README.md:3220) and every test use.

5. user as a bare third positional (sdk/event/event.js:171)

Repo convention for optional args is an options object — get(id, {user}), update(id, {isDelivered, user})
in this same file. parse(content, signature, user) breaks it; it should be parse(content, signature, {user}).

6. No tests

Diff is sdk/event/event.js only. Neither tests/testEvent.js nor testTypes/Event.test.ts gains a
positional case, a Buffer case, or a validation-error case. No CHANGELOG entry under ## [Unreleased]
either (CHANGELOG.md:15), which the repo keeps current.

Tree position

  • Branch is a fork synced by merging master in 112 times, so it carries merge commits — not mergeable as is per the owner's rule, independent of the code.
  • It is not merely "3 behind": head sits at v2.40.0 (9f71786, "Merge pull request #169 from starkbank/bump"), while master is past 2.41.0 and 2.42.0. The two-dot diff origin/master origin/pr-172 shows -1851 lines, i.e. merging it would revert master work: deletes sdk/merchantCard/log/log.js, sdk/merchantInstallment/log/log.js, the httpsAgent feature (tests/testHttpsAgent.js, testTypes/HttpsAgent.test.ts), PaymentRequest tax/darf support, the docstring fixes and the CHANGELOG [Unreleased] block.
  • Note: the local clone is shallow (.git/shallow present, master has 5 commits), so merge-base origin/master origin/pr-172 reports no merge base. That is a clone artifact, not a real unrelated history.

Rebase

Needs a rebase: yes. Trivial: no — replaying 313 commits with 112 merges is not the right move at all.
The branch should be thrown away and the single 12-line hunk re-applied on the current master tip
(cherry-pick d1b19a3 onto a fresh branch off 281ea55), which conflicts with nothing —
parse in sdk/event/event.js is byte-identical on master to the fork point.

Action to take

Yes, it is worth asking the author to squash onto master — but not as-is. Ask for a fresh branch off
master@281ea55 with one commit that:

  1. keeps parse({content, signature, user}) as the canonical form (starkinfra parity, .d.ts, README),
  2. accepts positional as the compat path, detected without swallowing Buffers — e.g. treat
    typeof content === 'string' || Buffer.isBuffer(content) as the positional case, or only
    destructure when signature === undefined && content.content !== undefined,
  3. throws error.InputError, not bare Error,
  4. updates types/event/event.d.ts to an overload, adds positional + Buffer tests to both
    tests/testEvent.js and testTypes/Event.test.ts, and adds a CHANGELOG.md [Unreleased] ### Fixed line.

If he would rather not carry it, the hunk is small enough to redo in-house; issue #163 is real (the
cryptic Unexpected token u in JSON is worth fixing) even though the author's diagnosis of the docs is not.

Two-line summary for the owner

One real 12-line hunk buried in a fork synced by 112 merge commits, sitting at v2.40.0 — merging it
would revert 2.41.0 and 2.42.0, so it cannot go in as is.
The hunk itself also regresses Buffer webhook bodies (the README's own Express snippet), flips a public
signature away from starkinfra/sdk-node parity without touching .d.ts, README or tests, and throws bare
Error instead of error.InputError — request changes and ask for one commit on the current master tip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] SyntaxError: Unexpected token u in JSON at position 0 in event.parse (v2.38.0)

2 participants