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

switch to 'cross-fetch' instead of 'request' #6

Merged
merged 7 commits into from
Jan 25, 2022

Conversation

strmer15
Copy link

  • Replaces request with cross-fetch, and isomorphic fetch library
    • The RequestOptions type is a mix of the browser and node-fetch options, so that server-side code can set things like the agent for keepalive, but it should have no effect in browser code
    • The lodash dependency was removed from the thrift-server-core library, because all it was used for was doing "deep merges" on the client, which weren't needed. Instead, a new function to merge together request options was added, with special logic for the Headers values (which can't utilize object spread).
    • The response.arrayBuffer() function is used to get an ArrayBuffer and convert the binary response into a Node Buffer, which is what the generated code expects to use when sending requests through the connection. We'll need to polyfill that with Webpack, but it should be just fine.
  • Removed all uses of the url.parse and url.format deprecated functions and replaced them with the WHATWG URL API
  • Fixed a number of tests and cleaned them up with the right response types

@strmer15 strmer15 requested review from a team January 19, 2022 18:27
Copy link

@anandkumarpatel anandkumarpatel left a comment

Choose a reason for hiding this comment

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

Looks good! just some minor questions.

Copy link

@markdoliner-doma markdoliner-doma left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review. Thanks for making these changes! It looks like there's a lot of good common-sense changes.

I'm not familiar with cross-fetch but I looked at it for a few minutes. Is it fair to say that it's a pretty small library, and it just proxies through to either a node implementation or a browser implementation depending on where it's running?

@strmer15
Copy link
Author

Looks good! just some minor questions.

Thanks for reviewing!

@strmer15
Copy link
Author

Sorry for the slow review. Thanks for making these changes! It looks like there's a lot of good common-sense changes.

Thanks for reviewing!

I'm not familiar with cross-fetch but I looked at it for a few minutes. Is it fair to say that it's a pretty small library, and it just proxies through to either a node implementation or a browser implementation depending on where it's running?

Yep, it just packages a browser implementation that uses the fetch library and then calls node-fetch when it's running on the server. See https://github.com/lquixada/cross-fetch#how-does-cross-fetch-work

…merging of options, adding unit tests for thrift-client, cleaning up some code
@strmer15
Copy link
Author

I'm going to merge so that I'm not blocked on the webpack 5 front anymore, but if anyone has any more comments or suggestions, I can address those in a separate PR. Thanks!

@strmer15 strmer15 merged commit baa0b58 into main Jan 25, 2022
@strmer15 strmer15 deleted the cplummer/TPS-2572/thrift_client_fetch branch January 25, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants