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

feat: Refactor Response class and add new initialization methods #28

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gaetschwartz
Copy link
Contributor

@gaetschwartz gaetschwartz commented Apr 20, 2023

Changes

  • Add default response status and status text in the Response constructors
  • Add Response.jsify method for initializing Response with a JS object as body
  • Separate Response.json method from Response.jsify for initializing Response with a JSON-encodable object as body
  • Add documentation for main the constructors of Response

Motivation

jsify is subject to minification and has somewhat bad performance. When using dart compile with minification enabled, the response json gets minified (after a certain depth AFAIK). It should not be the case. The new Response.json behaves simlarly from the previous implementation except that it doesn't minify as it uses json.encode. Renamed the previous implementation to Response.jsify as it could still be useful for some cases.

- Add default response status and status text in the `Response` constructors
- Add `Response.jsify` method for initializing `Response` with a JS object as body
- Separate `Response.json` method from `Response.jsify` for initializing `Response` with a JSON-encodable object as body
- Add documentation for main the constructors of `Response`
@Salakar Salakar requested a review from Ehesp May 2, 2023 10:37
@Ehesp
Copy link
Member

Ehesp commented May 2, 2023

@gaetschwartz thanks for this - what's the use case for the jsify method?

@gaetschwartz
Copy link
Contributor Author

Well, as jsify can translate Dart objects to Js objects directly, people could still have some use. As it was the old behavior I figured I would leave it.

@gaetschwartz gaetschwartz requested a review from Salakar May 14, 2023 12:34
@rrousselGit
Copy link
Contributor

I'm not really familiar with the project, but it looks alright at a quick glance.

I'd still consider this a breaking change (since the "json" constructor changes).
And it'd be great to have some tests. But the project doesn't seem to have some atm, so that's alright I guess

Co-authored-by: Remi Rousselet <darky12s@gmail.com>
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