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

Replace superagent with fetch #236

Open
doug-wade opened this issue May 31, 2016 · 3 comments
Open

Replace superagent with fetch #236

doug-wade opened this issue May 31, 2016 · 3 comments
Labels
enhancement New functionality.

Comments

@doug-wade
Copy link
Collaborator

doug-wade commented May 31, 2016

Seems like fetch is rapidly becoming the standard, with pretty good polyfills for both client and node. Swapping out superagent for fetch would reduce the amount of documentation required for data loading: rather than sending in a loader argument to the page API and then explaining superagent, we'd just say "use fetch like you already do".

We might also consider a "bring your own" approach.

Migrated from #50

@gigabo gigabo added enhancement New functionality. breaking change labels Jun 1, 2016
@aickin
Copy link
Contributor

aickin commented Jun 6, 2016

I'd like to bump this feature request, because I'm curious how folks feel about it as a future direction. Would you accept a PR with it? Do you think it's a step forward or a step back?

Also, if you do think it's a good idea, I'm curious if you'd want it to be opt-in or opt-out. Right now client code opts in to the ReactServerAgent cache by requiring ReactServerAgent. If we instead intercepted all calls to fetch, it's possible that we could end up sending down some secret information in the cache that was not intended to be sent to the client. This could be alleviated by making client code opt in by doing something like import fetch from "react-server/fetch", but that would require documentation and is the kind of thing developers miss. Thoughts?

@gigabo
Copy link
Contributor

gigabo commented Jun 6, 2016

Could we separate the data bundling (ReactServerAgent.cache) from the request library? Give it a clean API so that a fetch wrapper/interceptor could talk to it as easily as a superagent wrapper?

Then, I could do import SuperAgent from "react-server-superagent" or import fetch from "react-server-fetch" depending on my preference. And it would be easy to add support for other libraries.

that would require documentation and is the kind of thing developers miss.

Yeah, that's a concern. It has the advantage of being explicit, though, so the potential for surprise is lower once you know about it. But it doesn't have the same magic factor of "hey, this just works!"

@roblg
Copy link
Member

roblg commented Jun 6, 2016

I'm not personally married to the idea of ReactServerAgent/anything-built-on-superagent. The builder pattern makes writing tests a bit of a PITA.

I think provided we maintain feature compatibility (and at least have an upgrade path) there's no reason we wouldn't entertain the idea of switching? In particular, I think it'd be nice to have plugRequest and plugResponse continue working, or at least be possible. Since fetch is such a simple API, maybe just allow users of react-server to do something like

const RS = require("react-server");
RS.setFetchImpl(myFetchImplFunction)

I know we're using ReactServerAgent internally for file uploads in at least one scenario, so it'd be good for that path to keep working as well (seems like it would, since github/polyfill seems to support it)

I also agree with the things @gigabo said, I just type slower so he beat me to them. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality.
Projects
None yet
Development

No branches or pull requests

4 participants