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

rewrite language-http #373

Merged
merged 70 commits into from
Jan 11, 2024
Merged

rewrite language-http #373

merged 70 commits into from
Jan 11, 2024

Conversation

mtuchi
Copy link
Collaborator

@mtuchi mtuchi commented Aug 30, 2023

Summary

Re-write language-http to use the new common helpers. Drops support for axios.

This introduces many breaking changes.

Closes #316

Changes to http:

  • Remove axios export and axios APIs used
  • Use new request from common/util
  • Use new expandReference from common/util
  • map and fix existing unit tests
  • support TLS (cert details on config)
  • Add auto parsing request body on 7463dbf, Fixes new common-http: can't post JSON #377 JC WHAT IS THIS
  • write clean new response object to state
  • throw clean errors
  • drop support for cookie handling with tough-cookies
  • drop support for gzip

Also note some changes to common:

  • Add a status message along side a status code
  • Use statusMessage and statusCode instead of message and code

QA Notes

Things we need to test in real job code:

  • Each of the major handlers - including sending json data
  • Error handling - logs and error objects
  • Forms and redirects
  • Log output generally
  • Ensure references are handled correctly

I'd also love to test some datasources which have real auth

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • Are there any unit tests? Should there be?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.

@mtuchi mtuchi linked an issue Aug 30, 2023 that may be closed by this pull request
@mtuchi mtuchi changed the title 316 http rewrite rewrite language-http Aug 31, 2023
@mtuchi mtuchi changed the base branch from main to 314-common-new-http-helpers August 31, 2023 06:49
Base automatically changed from 314-common-new-http-helpers to epic/http August 31, 2023 11:48
@josephjclark
Copy link
Collaborator

josephjclark commented Sep 1, 2023

Comments from our discussion:

  • Ok, let's break everything in favour of a nice clean API
  • We need to keep statusText, if we can. So we need to change the underlying response object to { code, headers, body, message }
  • In tests, write a bunch of general tests under get (ie, test auth, baseurl, state etc). For post, put etc, you should a) make sure there is at least one test (even if it's boring!) and b) only add more tests if they do something new. For example, for post you'll want to test the body - but don't duplicate those tests to the other handlers. Basically we're using get as a proxy for request, and we're minimally testing the other functions
  • I don't think we need to support gzip any more - undici should handle this for us. Remove support for the property and tests. We'll add a gzip flag later if we need it.
  • We need to handle maxRedirections. By default unidici won't follow redirects. We can either continue to maintain followAllRedirects (which hopefully means we can set maxRedirections to Infinity), or we replace it with maxRedirections and just feed it through to Undici. I would prefer followAllRedirects.
  • We can replace successCodes with errorMap: { 404: false } to not throw on 404. This needs implementing in the underlying request helper
  • Cookies - do we need to keep the cookie stuff? We've left the old functionality but I think that was axios specific? We need a real test case really
  • We will need to support a form param, which take as a JSON object. We need to encode this properly and set the headers. We should support this down at the request helper level. We may also be able to support a JSON body with multipart form content type header. Does unidici do this for us?
  • Add unit test to common to post, patch and put in client.request. Each request includes a JSON object as the body. No headers. Unit tests should ensure that that undici properly encodes and sends the JSON to the server.

@josephjclark
Copy link
Collaborator

@mtuchi I have spun #377 out of the post issue. I suggest you discuss with Stu/Taylor next week and raise a fix to the underlying helper (you can do it on this branch)

@mtuchi

This comment was marked as resolved.

@josephjclark

This comment was marked as resolved.

@josephjclark

This comment was marked as resolved.

@josephjclark

This comment was marked as resolved.

@josephjclark

This comment was marked as resolved.

@josephjclark

This comment was marked as resolved.

@mtuchi mtuchi marked this pull request as ready for review October 6, 2023 13:38
@mtuchi mtuchi requested a review from josephjclark October 6, 2023 13:39
@mtuchi mtuchi mentioned this pull request Oct 6, 2023
@josephjclark

This comment was marked as resolved.

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Some comments here in addition to the logging stuff.

Also, documentation! We've got a bunch of stuff now like parseAs and errorMap which we feed through to the underlying request. None of that is obvious, it's all new, an we can't just say "oh it's like axios" anymore.

So we're going to have to work out a documentation solution here. I'll have to open an issue for that.

.changeset/red-cherries-yawn.md Outdated Show resolved Hide resolved
.changeset/rude-penguins-worry.md Outdated Show resolved Hide resolved
packages/common/src/util/http.js Outdated Show resolved Hide resolved
packages/common/src/util/http.js Show resolved Hide resolved
packages/http/src/Adaptor.js Outdated Show resolved Hide resolved
packages/http/src/Adaptor.js Outdated Show resolved Hide resolved
packages/http/src/Adaptor.js Outdated Show resolved Hide resolved
packages/http/src/Utils.js Outdated Show resolved Hide resolved
packages/http/src/Adaptor.js Outdated Show resolved Hide resolved
mtuchi and others added 5 commits November 1, 2023 10:43
@josephjclark

This comment was marked as resolved.

@josephjclark
Copy link
Collaborator

josephjclark commented Jan 11, 2024

Some changes I've instigated in common:

  • Adjust url from the response to be bit simpler
  • Rename status and message on the response to statusCode and statusMessage, just because message is too ambiguous to be useful

Changes instigated in http:

  • Restructure of the unit tests
  • Move the request helper into Utils. Now Adaptor.js only contains operations, which is neat.

@josephjclark

This comment was marked as resolved.

@josephjclark josephjclark merged commit b8ade60 into epic/http Jan 11, 2024
1 check passed
@josephjclark josephjclark deleted the 316-http-rewrite branch January 11, 2024 16:30
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.

http: rewrite to use new helper functions
2 participants