fix: Cannot assign to read only property 'clone'

[magne4000]

On node@18, overriding res.clone or req.clone can cause the following error:

TypeError: Cannot assign to read only property 'clone' of object '#<_Request>'
TypeError: Cannot assign to read only property 'clone' of object '#<_Response>'

~~Using Object.defineProperty instead fixes this issue.~~

Applying this patch when running on Bun fixes this issue.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
webroute	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 18, 2024 4:48am

[magne4000]

Or perhaps clone method override should only be applied when running through bun?

[magne4000]

Seems like using Object.defineProperty fails with bun:

TypeError: Attempting to change the getter of an unconfigurable property

Should we then just wrap fixResponseClone usage in an if (typeof Bun !== "undefined") { ... } block?

[sinclairnick]

@magne4000 Thanks for pointing this out. I've been using this with Bun, hence the initial "fix" (that broke node).

I think adding a Bun guard is a good idea. Do you want to add the condition on this branch? Happy to create another PR otherwise

[magne4000]

Tests are passing (node@18, node@20 and [email protected])

• new isBun util
• Added somes fixes to support node@18:
  • updated valibot dependency
  • replaced tsup-node by tsup (which is the official binary name)

[sinclairnick]

Hey @magne4000 out of interest, did you come across the node issues by running a server or by running the tests?

Funnily enough I have a pending issue #33 for exactly the problems which you've helped solve here, but I'm not 100% certain on how I'll go about attacking it. A CI testing matrix + unit tests might suffice. But the more extreme version would be to setup e2e tests too(/instead of).

Interested to hear if you have any thoughts here.

[magne4000]

I had the tsup issue while running build:packages, and the valibot one while running test.

A CI testing matrix + unit tests might suffice. But the more extreme version would be to setup e2e tests too(/instead of).

Starting by a matrix with unit tests (and builds) will already solve most of the possible regressions. e2e could come later, using playwright or simply running dev servers + local fetch requests.

[sinclairnick]

I've added a (partial) matrix (currently just node versions x OS). I've also added failing tests for the issues this PR solves, which should pass once it's all merged in.