search

parroty / exvcr

HTTP request/response recording library for elixir, inspired by VCR.
MIT License
724 stars 132 forks source link

Use Jason Instead of JSX #206

Closed ruslandoga closed 2 months ago

ruslandoga commented 1 year ago

Continues from https://github.com/parroty/exvcr/pull/179

👋 @parroty

I wonder what you think about using Jason instead of jsx

barttenbrinke commented 1 year ago

This looks good @ruslandoga , maybe @parroty can merge it ? 👍

parroty commented 1 year ago

Thank you for the PR 🙇 . Do you have any insights around compatibility implications with this change? I am just wondering if there's any corner cases that can break by changing the JSON engine 🤔 .

ruslandoga commented 1 year ago

👋 @parroty

Yes, it very well might break something since jsx and Jason have different defaults. For example, the previous attempt to use Jason was blocked by Jason being more strict and not encoding invalid bytes like jsx https://github.com/parroty/exvcr/pull/179#discussion_r702496892

There might be other differences that weren't uncovered by the tests.

parroty commented 1 year ago

The breakage on certain conditions are concerning part of this change, though I understand the concern mentioned in the previous issue. Describe the procedure (for re-recording?), or something...? 🤔

ruslandoga commented 1 year ago

Also https://github.com/parroty/exvcr/issues/153 lists some concerns people have about depending on jsx through exjsx

I'll try out this branch (exvcr + jason) tomorrow on https://github.com/plausible/analytics and report back if anything breaks.

ruslandoga commented 1 year ago

In Plausible all VCR tests pass without any re-recording: https://github.com/ruslandoga/analytics/pull/147

```console $ mix test --include slow 13:54:33.063 [info] Migrations already up 13:54:33.091 [info] Migrations already up Including tags: [:slow] ............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................. Finished in 123.2 seconds (16.4s async, 106.7s sync) 9 doctests, 1396 tests, 0 failures ```

I had to update Finch version since ExVCR mocks request!/2,3 functions which were not present in Finch 0.14.

barttenbrinke commented 1 year ago

Imho even though it might break things downstream, keeping this unsupported dependency would also be a reason for people to drop exvcr.

parroty commented 1 year ago

Thank you for the feedbacks / comments. I am thinking to merge for v0.15.0 (after releasing other ones as v0.14.x). Please allow some time to test on my other libraries too.