exvcr icon indicating copy to clipboard operation
exvcr copied to clipboard

Use Jason Instead of JSX

Open ruslandoga opened this issue 1 year ago • 8 comments

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

  • [x] rebase
  • [x] minimal changes wrt master

👋 @parroty

I wonder what you think about using Jason instead of jsx

ruslandoga avatar Jun 23 '23 13:06 ruslandoga

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

barttenbrinke avatar Aug 20 '23 09:08 barttenbrinke

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 🤔 .

parroty avatar Aug 24 '23 13:08 parroty

👋 @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.

ruslandoga avatar Aug 24 '23 13:08 ruslandoga

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...? 🤔

parroty avatar Aug 24 '23 13:08 parroty

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 avatar Aug 24 '23 13:08 ruslandoga

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

$  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.

ruslandoga avatar Aug 25 '23 07:08 ruslandoga

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

barttenbrinke avatar Aug 25 '23 07:08 barttenbrinke

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.

parroty avatar Aug 27 '23 05:08 parroty