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

Delete the Naive JSON codec #214

Closed
wants to merge 1 commit into from
Closed

Conversation

lhchavez
Copy link
Contributor

Why

We don't use this and it's only making our test harness slower.

What changed

This change drops the NaiveJsonCodec, which is slower and more verbose.

Versioning

  • Breaking protocol change
  • Breaking ts/js API change

@lhchavez lhchavez requested a review from a team as a code owner June 18, 2024 20:27
@lhchavez lhchavez requested review from Monkatraz and removed request for a team June 18, 2024 20:27
@jackyzha0
Copy link
Member

personally I find it really helpful when debugging tests to use JSON mode so i can inspect the raw payload more easily

imo ok to keep around

We don't use this and it's only making our test harness slower.

This change drops the `NaiveJsonCodec`, which is slower and more
verbose.
@lhchavez
Copy link
Contributor Author

personally I find it really helpful when debugging tests to use JSON mode so i can inspect the raw payload more easily

imo ok to keep around

imo we shouldn't be testing code that is not being shipped, since it slows everyone down (don't pay for what you don't use!).

if it's about ease of inspection and debugging, we should do something different for that.

@jackyzha0
Copy link
Member

i don't think it considerably slows any thing down? CI is maybe ~5-8s faster but its already fast enough that I don't really notice it

we also dont pay for CI as this is an open source repo

@lhchavez
Copy link
Contributor Author

i don't think it considerably slows any thing down? CI is maybe ~5-8s faster but its already fast enough that I don't really notice it

we also dont pay for CI as this is an open source repo

i constantly have to fight against the matrix of tests that need to be re-run every time i make one change in one test case. those 8s and ~40 extra lines of output add up 🙃. how about we make debugging easier? we can probably make some convenience functions to decode webpack into JSON when things fail.

@jackyzha0
Copy link
Member

closing for now, we can reconsider later :)

@jackyzha0 jackyzha0 closed this Jul 12, 2024
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

3 participants