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

[PROP]: Use node-fetch as standard server-side request module #3

Open
HoukasaurusRex opened this issue Jul 10, 2020 · 5 comments
Open
Labels
type:proposal Documentation change proposal

Comments

@HoukasaurusRex
Copy link
Contributor

Why node-fetch?

Why Not ____?

  • request is deprecated
  • isomorphic fetch uses node-fetch under the hood, acting more like a client polyfill with ss support
  • axios is a good module as well, but it would need to be implemented on client (11kb) to be API-consistent with client request module
@HoukasaurusRex HoukasaurusRex added the type:proposal Documentation change proposal label Jul 10, 2020
@TNieminen
Copy link
Contributor

TNieminen commented Aug 19, 2020

We are already using Axios as the provider of our request.js implementation. The reason that I can remember (that we briefly discussed recently) was that Axios provides a progress interface. I think @codebender828 might have additional comments on this because if I remember correctly he was originally quite strongly on team Axios.
From my point of view I am okay with both node-fetch and Axios. I might prefer Axios a bit because we are already using it on the frontend.

@HoukasaurusRex
Copy link
Contributor Author

Fetch, being a native browser successor of the decades-old xhr interface, is not only good for its size (no wrappers needed), features exclusive to it like the cache api and req aborting, but also as of this year now supports req/res low level upload streams, whereas xhr actually needs to buffer the response to memory. Full support is beginning to roll out across all browsers (and this is easy to detect support for and fallback)

@HoukasaurusRex
Copy link
Contributor Author

This proposal is also scoped to server-side javascript, which can have different requirements from client side HTTP requesting. The first priority is having a single consistent request interface to use across a single system (so one repo absolutely should not be using multiple request modules), but there can be benefits in developer overhead in also standardizing the request interface across systems as well

@TNieminen
Copy link
Contributor

On the topic of request aborting

Point 1. Axios can also do request aborting and we are doing this already in request.js.

On the topic of cache

Point 2 As for cache, I'm actually not that familiar with cache policies in fetch. I assume it is doing something more than just modifying the cache-control header? Because ofc on Axios we can also set headers in the request. In addition to this there seems to be some kind of adapter for Axios:
related discussion
related adapter

However even if the adapter solves all the issues, there seems to be a issue with performance in node which would require us to do some additional configuration:

axios-cache-adapter was designed to run in the browser. It does work in nodejs using the in memory store. But storing data in memory is not the greatests idea ever.

You can give a store to override the in memory store but it has to comply with the localForage API and localForage does not work in nodejs for very good reasons that are better explained in this issue.

The better choice if you want to use axios-cache-adapter server-side is to use a redis server with a RedisStore instance as explained above in the API section.

Question 1: Also, are you certain node-fetch gives the same functionalities as browser fetch?
The documentation seems to imply otherwise?

Due to the nature of Node.js, the following properties are not implemented at this moment:

type
destination
referrer
referrerPolicy
mode
credentials
cache
integrity
keepalive

Question 2: Also in which cases would we need cache for requests in server side code? We are not requesting a lot of static data from the server?

On the topic of progress

Question 3: Do we see ourselves needing progress on the serverside?

Others comments

Whichever we end up choosing, maybe we should create a request wrapper on the server side similar to that of webapp. This way we don't have to marry ourselves into any specific implementation

@HoukasaurusRex
Copy link
Contributor Author

HoukasaurusRex commented Aug 20, 2020

Point 2 As for cache, I'm actually not that familiar with cache policies in fetch. I assume it is doing something more than just modifying the cache-control header? Because ofc on Axios we can also set headers in the request. In addition to this there seems to be some kind of adapter for Axios:

I mean specifically the Cache API

Question 1: Also, are you certain node-fetch gives the same functionalities as browser fetch?

I said "API-Consistent", which it is. If a developer is trying to use fetch options like "keepalive", "referrer", or "credentials" , they most likely don't know what they mean or do.

Question 2: Also in which cases would we need cache for requests in server side code? We are not requesting a lot of static data from the server?

Question 3: Do we see ourselves needing progress on the serverside?

No, these are responding to its viability on the client. See the original issue text for serverside implementation reasoning.

Note: this wasn't about progress though, it was about streams, which is much broader and much more powerful than just general http req progress indicators buffered to memory.

Whichever we end up choosing, maybe we should create a request wrapper on the server side similar to that of webapp. This way we don't have to marry ourselves into any specific implementation

That's a good idea, but we should decide on a module now. I would want to commit to fetch (node-fetch on server) for the reasons above as well as the speed of development happening on the fetch api and the probable deprecation of xhr in the future, or at least the lack of development on it now other than general maintenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:proposal Documentation change proposal
Projects
None yet
Development

No branches or pull requests

2 participants