Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Jason Instead of JSX #206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ruslandoga
Copy link

@ruslandoga ruslandoga commented Jun 23, 2023

Continues from #179

  • rebase
  • minimal changes wrt master

👋 @parroty

I wonder what you think about using Jason instead of jsx

@ruslandoga ruslandoga force-pushed the use-jason-ruslan branch 2 times, most recently from bdf4d26 to 1498944 Compare June 23, 2023 13:27
jherdman and others added 3 commits June 23, 2023 21:33
Given that the broader Elixir community is circling their wagons around
Jason, and issue parroty#153, it seems that moving to Jason would be a wise
choice.
@barttenbrinke
Copy link

barttenbrinke commented Aug 20, 2023

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

@parroty
Copy link
Owner

parroty commented Aug 24, 2023

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
Copy link
Author

ruslandoga commented Aug 24, 2023

👋 @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 #179 (comment)

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

@parroty
Copy link
Owner

parroty commented Aug 24, 2023

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
Copy link
Author

ruslandoga commented Aug 24, 2023

Also #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
Copy link
Author

ruslandoga commented Aug 25, 2023

In Plausible all VCR tests pass without any re-recording: ruslandoga/plausible#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.

@barttenbrinke
Copy link

barttenbrinke commented Aug 25, 2023 via email

@parroty
Copy link
Owner

parroty commented Aug 27, 2023

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.

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.

None yet

4 participants