search

contiamo / restful-react

A consistent, declarative way of interacting with RESTful backends, featuring code-generation from Swagger and OpenAPI specs 🔥
MIT License
1.87k stars 109 forks source link

Extract usage of AbortController into own hook to check if it is available #243

Closed mbrookson closed 4 years ago

mbrookson commented 4 years ago

This provides a hook that works regardless of whether AbortController API is available, for example if the code is running in the browser vs in static html build process.

Fix for https://github.com/contiamo/restful-react/issues/242

Would appreciate another person's manual verification if possible!

mbrookson commented 4 years ago

@fabien0102 Looks like Travis CI might need a manual restart? Tests should pass again now but seems to be stuck still.

TejasQ commented 4 years ago

DO NOT MERGE THIS because I need to show this to @github

TejasQ commented 4 years ago

FYI –– I disabled Travis and run tests on Netlify now so if Netlify is green on future PRs, it means tests pass. I did this because Travis is unreliable and hangs too often.

mbrookson commented 4 years ago

DO NOT MERGE THIS because I need to show this to @github

Is this because of a Travis issue?

TejasQ commented 4 years ago

Is this because of a Travis issue?

@mbrookson, no. It's a GitHub issue. They don't consider PRs approved on mobile devices even though they are.

Also, while the tests pass, it seems we're leaking memory in Get according to this PR:

image

TejasQ commented 4 years ago

I don't see that memory leak locally though and it may not be a big deal. I'm more concerned with GitHub.

The issue I'm trying to help solve is this one. Notice how even though the PR is approved, it will not permit a merge.

ezgif com-resize (2)

Unfortunately, this GIF and video reproduction isn't enough for GitHub to realize it's an issue. They asked for a live PR, and this PR happens to serve that purpose. It'll lose that purpose when we merge it, hence my "do not merge".

TejasQ commented 4 years ago

Ah, nevermind. All passing checks render it mergeable on my phone. If we had one pending check, the issue would have persisted.

Thanks for your contribution, @mbrookson!