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

Abortable requests #34

Closed
6 tasks done
Jyrno42 opened this issue Nov 22, 2018 · 7 comments
Closed
6 tasks done

Abortable requests #34

Jyrno42 opened this issue Nov 22, 2018 · 7 comments
Assignees
Milestone

Comments

@Jyrno42
Copy link
Contributor

Jyrno42 commented Nov 22, 2018

I think adding support for Aborting requests requires some more planning due to the differences between fetch and superagent in how they deal with aborting requests.

In fetch one needs to provide an AbortController.signal in the arguments to the fetch call. The controller itself has a method abort which can be called to cancel the request. When a request is cancelled it's promise is rejected.

However, with superagent - abort is a method on the request. Upon abort the promises behaves similarly to fetch. It actually doesn't, we need to add a listener to abort event and reject via that.

Based on this, I have a couple of questions that need to get answered before we agree upon the final API for abortable requests.

  1. Should we mimic fetch style api or go with something else entirely?
  2. Can we have the same api regardless of the backend (fetch vs superagent)?
  3. Can we still keep our Resource stateless?
  4. How would this affect saga-resource?

Progress

  • Agree on an Api
  • Implementation for fetch
  • Implementation for superagent
  • Test in react-native
  • Documentation update
  • Saga-resource integration
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Nov 22, 2018

for reference rgs. react-native: facebook/react-native#18115

E: Also, this workaround might be needed: facebook/react-native#18115 (comment)

Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Nov 26, 2018
- Works & Tested for fetch
- TODO: Superagent compatibility and tests

Fixes thorgate#34
Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Nov 26, 2018
- Works & Tested for fetch

Fixes thorgate#34

TODO:

- Superagent compatibility and tests
- Check if cross-fetch provides form-data or not
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Nov 26, 2018

Got it working with fetch backend. Will try to get superagent to work with the same tg-resources API soon. Note, I also need to test this on react-native to know whether we need the workaround or not.

Had to swap out isomorphic-fetch with cross-fetch since we needed an updated version of node-fetch however isomorphic-fetch seems to be abandoned.

Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Nov 27, 2018
Works & Tested for fetch and superagent, both using the same
external API.

- Replaces isomorphic-fetch with cross-fetch since the latter
   is maintained. Sadly no form-data in cross-fetch so we still need
   to keep form-data things in our dependencies.

Fixes thorgate#34
Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Nov 27, 2018
Works & Tested for fetch and superagent, both using the same
external API.

- Replaces isomorphic-fetch with cross-fetch since the latter
   is maintained. Sadly no form-data in cross-fetch so we still need
   to keep form-data things in our dependencies.

See thorgate#34
Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Nov 27, 2018
Works & Tested for fetch and superagent, both using the same
external API.

- Replaces isomorphic-fetch with cross-fetch since the latter
   is maintained. Sadly no form-data in cross-fetch so we still need
   to keep form-data things in our dependencies.

See thorgate#34
@Jyrno42 Jyrno42 added this to the 3.1.0 milestone Nov 27, 2018
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Nov 27, 2018

Superagent now also works

Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Nov 27, 2018
Works & Tested for fetch and superagent, both using the same
external API.

- Replaces isomorphic-fetch with cross-fetch since the latter
   is maintained. Sadly no form-data in cross-fetch so we still need
   to keep form-data things in our dependencies.

See thorgate#34
Jyrno42 added a commit to Jyrno42/tg-resources that referenced this issue Dec 26, 2018
Works & Tested for fetch and superagent, both using the same
external API.

- Replaces isomorphic-fetch with cross-fetch since the latter
   is maintained. Sadly no form-data in cross-fetch so we still need
   to keep form-data things in our dependencies.

See thorgate#34
@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Dec 31, 2018

Sad news about react-native, currently I did not manage to get aborting to work. See my testing repo here: https://github.com/Jyrno42/rn-tg-resources-tester. The following commit contains a testcase for aborting a request on react native and some explanations on what I tried to get aborting to work

Jyrno42/rn-tg-resources-tester@58f81ed

@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Jan 31, 2019

We should do a release without RN support. We should also try to advocate (or do it ourselves) the AbortController thing inside rn core.

@jorgenader
Copy link
Contributor

Released in v3.1.0

@Jyrno42
Copy link
Contributor Author

Jyrno42 commented Feb 4, 2019

Some progress on react-native support - got it working with abortcontroller-polyfill (see #48). Once that is merged I would like to do a new release.

After that I would focus my efforts towards getting facebook/react-native#18115 resolved. Seemed to get it working by updating whatwg-fetch inside react-native core manually and using import 'abortcontroller-polyfill/dist/abortcontroller-polyfill-only' but I have to do a bit more testing with that. I also need to put back the changes to whatwg-fetch that are done in react-native master before I can submit a PR about it.

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

No branches or pull requests

2 participants