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: add ability to execute raw (untyped) requests #3

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

btrautmann
Copy link
Contributor

@btrautmann btrautmann commented Jul 18, 2023

📰 Summary of changes

What is the new functionality added in this PR?

This PR adds RawRequest as a subclass of NetworkRequest which is a convenience for creating requests whose methods are not known until runtime. This is the case for RetryInterceptor in the test_track_dart_client (see PR here). Rather than switching on the method of the failed request and creating a request-per-type (where all params other than the method are identical), RawRequest allows us to pass the type in as a parameter. Very open to other implementations!

🧪 Testing done

What testing was added to cover the functionality added in this PR

Since the underlying data passed to the request type is pre-existing, I didn't add any tests and I think the existing SturdyHttp tests are sufficient.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 2.85% and project coverage change: -1.37 ⚠️

Comparison is base (12ba3aa) 29.72% compared to head (bddd49e) 28.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #3      +/-   ##
==========================================
- Coverage   29.72%   28.36%   -1.37%     
==========================================
  Files           9        9              
  Lines         656      691      +35     
==========================================
+ Hits          195      196       +1     
- Misses        461      495      +34     
Impacted Files Coverage Δ
lib/src/network_request.dart 54.54% <0.00%> (-12.13%) ⬇️
lib/src/sturdy_http.dart 97.80% <0.00%> (-1.09%) ⬇️
lib/src/network_request.freezed.dart 9.61% <3.12%> (-2.89%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@willlockwood willlockwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few little questions for ya :D

domain tafn

Comment on lines +181 to +183
/// A raw body. Allows for nullable untyped data that is passed directly
/// to `data` of the request, useful for instances where the data type
/// is not known until runtime.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth noting how data will be treated? As in, do we just call toString() on it and send it as the body? Does it have to conform to something that can be .toJsoned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does:

nullable untyped data that is passed directly to data of the request

not help?

Dios data is untyped (Object?) so the type here can be left as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps related to Will's line of questioning: does the usage of the .json constructor here result in setting the content-type header to application/json whereas this type will result in not setting the header? the reason i ask is because technically if we're sending json we are supposed to set that header and if we're sending something else we should set the corresponding header for that data type (form-url-encoded, mutlipart, application/xml, whatever)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do any of that (mainly because Dio does it for you). It may have been that I originally planned to, but then learned Dio did it (hard to recall now). If we don't feel there's a big enough value-add from the type safety/readability offered by these (I don't imagine consumers are passing NetworkRequestBodys around, so type safety is maybe not a huge deal here), we could nix the type entirely and just allow Object? 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, i like having the type safety for specifying the interface. I wouldn't won't to accept Object? where a more constrained type would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth calling out that we actually remove the implied content type interceptor by default, so consumers of sturdy need to do this themselves. I should probably add documentation on the NetworkRequestBody type at the very least indicating that this doesn't dictate content type deduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up PR here: #4

lib/src/network_request.dart Outdated Show resolved Hide resolved
lib/src/network_request.freezed.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@willlockwood willlockwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domain lgtm

@btrautmann
Copy link
Contributor Author

/no-platform

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.

4 participants