fix: handle network error in retry function and add a callback #37

ntelkedzhiev · 2018-09-17T07:44:19Z

Closes #36

ntelkedzhiev · 2018-09-17T08:08:22Z

@nodkz Should forceRetry and onRetry be combined in something like onRetry(forceRetry, delay, attempt)? Also, do you want me to only handle the Network request failed error or all error that do not contain a response from the server as it is now.

nodkz · 2018-09-17T08:42:15Z

@ntelkedzhiev can you fix broken tests?
Also, run yarn lint it also throws indentation errors (tab -> 2 spaces).

Run yarn test or npm test to run all tests locally before committing.

Should forceRetry and onRetry be combined in something like onRetry(forceRetry, delay, attempt)?

While reviewing your code I came with the same idea about improvement of onRetry method.
Let's do it! 👍
But add more functionality to it - onRetry(meta: { forceRetry: Function, delay: number, attempt: number, lastError: Error }): false | any:

make one argument meta, with 4 properties (it allows to add more props in future without care of its order)
if this method returns false then abort requests, otherwise for any other returned value (even null, undefined) middleware should continue requests.

forceRetry is unobvious and looks like some piece of 💩 BUT forceRetry needs to leave as is for backward compatibility and mark as DEPRECATED in readme.md.

Also, do you want me to only handle the Network request failed error or all error that do not contain a response from the server as it is now.

For now, I don't know what behavior will be better. So do it according to your real use cases.

ntelkedzhiev · 2018-09-17T08:46:51Z

Sounds good! Thanks for the suggestions! Will do.

ntelkedzhiev · 2018-09-17T11:04:18Z

@nodkz When I run yarn test I get:

Requires Babel "^7.0.0-0", but was loaded with "6.26.3". If you are sure you have a compatible version of @babel/core, it is likely that something in your build process is loading the wrong version. Inspect the stack trace of this error to look for the first entry that doesn't mention "@babel/core" or "babel-core" to see what is calling Babel.

Any idea why?

nodkz · 2018-09-17T12:22:42Z

It's very strange.
Try to remove node_modules folder and install packages from scratch yarn install:

rm -r ./node_modules
yarn install

ntelkedzhiev · 2018-09-17T12:43:37Z

I'm now getting:

 Jest encountered an unexpected token
  ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){
import fetchMock from 'fetch-mock';
^^^^^^
SyntaxError: Unexpected token import

nodkz · 2018-09-17T13:46:12Z

I have just clone your repo, and all works good on MacOS:

git clone [email protected]:ntelkedzhiev/react-relay-network-modern.git ./_fork_rrnm

yarn install 
# it returns some warnings with `fsevents`, but just ommit them

yarn test
# returns following output

What operation system do you use?

nodkz · 2018-09-17T13:49:19Z

What operation system do you use?

Your error looks like that babel was resolved incorrectly by node.

Maybe you have globally installed babel?
Try to remove it

npm uninstall -g babel
npm uninstall -g babel-cli
npm uninstall -g jest

ntelkedzhiev · 2018-09-17T14:33:49Z

Very weird. No, I don't have any of the babel/jest packages globally installed. Tried it on two Macs - running node 9.11.2 and 10.10.0 and it didn't work. Not sure how to fix this. What version of node are you running?

I also don't get any fsevents errors.

nodkz · 2018-09-17T14:47:48Z

What yarn version do you use?

yarn --version

Mine is 1.7.0

Just updated to the latest version 1.9.4

curl -o- -L https://yarnpkg.com/install.sh | bash

And try again it with your repo to run tests. And it works without babel error.

nodkz · 2018-09-17T14:48:53Z

If you update yarn, you'll need to remove node_modules folder and install packages again via yarn install.

ntelkedzhiev · 2018-09-17T15:00:03Z

I'm running 1.9.4 as well.

ntelkedzhiev · 2018-09-17T15:08:43Z

Maybe this is needed: jestjs/jest#6053 (comment).

I am also able to successfully build which is weird too.

nodkz · 2018-09-17T15:12:21Z

Damn. First time met such problem.

Can you confirm that parent folders do not contain .babelrc files?
Maybe they somehow overrides settings from .babelrc in this repo?!

ntelkedzhiev · 2018-09-17T15:29:58Z

Yes, no .babelrc files in parent folders.

nodkz · 2018-09-17T15:42:21Z

Just updated packages in master, can you merge this changes to your repo.

Tried to do it directly to your repo, but it looks like that you do not allow (remove checkbox when open PR) to send commits from maintainers/contributors.

After merging with master, it required to run yarn install for updating packages. Maybe it solves a problem, maybe not.

…etwork-modern into retry-handle-network-error

ntelkedzhiev · 2018-09-18T12:53:42Z

@nodkz Didn't have success after you pushed the changes. I added babel.config.js (and removed .babelrc) as documented here: https://github.com/facebook/jest/blob/master/docs/GettingStarted.md#using-babel.

Also fixed the error so PR is ready to merge. Let me know what you want to do with the babel issue since it is included in this PR and whether you want me to make any further changes.

nodkz · 2018-09-18T13:11:29Z

Please return back all babel configs, they are required for package building for the different environments.

Just copy all content from https://github.com/relay-tools/react-relay-network-modern/blob/master/.babelrc to babel.config.js.

nodkz · 2018-09-18T17:14:55Z

This code has some bug in tests.

When running tests in watch mode:

yarn watch

and re-run tests several times, it sometimes throws an error, sometimes pass.

It's a very strange behavior. Try to review it tomorrow.

nodkz · 2018-09-18T17:23:46Z

and I also think that the code is dirty and middlewate should be completely refactored.

Killed about 1 hour on testing and understanding what happens. It's bad sign.

ntelkedzhiev · 2018-09-19T07:48:29Z

Agreed. It needs to be rewritten. I'll submit a reworked PR next week.

nodkz · 2018-09-19T08:12:02Z

In progress right now

nodkz · 2018-09-19T13:47:29Z

@ntelkedzhiev Done. Please test it with your app.

NOTABLE CHANGE: Property onRetry was renamed to beforeRetry. It's a better name because it allows to abort following retry or run it immediately.

Pull latest changes from ntelkedzhiev/react-relay-network-modern
Run

yarn build

Copy generated lib, es, mjs, node8 folders to your app's node_modules/react-relay-network-modern folder.
Launch your app

nodkz · 2018-09-19T16:01:48Z

My coworkers tested the new version with their app. All works well.
So you may try the published version from npm and if you find an error, please let me know.

Thanks for your help!

nodkz · 2018-09-19T16:02:15Z

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ntelkedzhiev · 2018-09-20T08:13:26Z

@nodkz Great! Thanks a lot man!

ntelkedzhiev added 3 commits September 17, 2018 10:30

fix: handle network error in retry function and add a callback

71621f9

Update README.md

9a9b476

Update README.md

ebd347f

Improve OnRetry function

2a83072

Formatting

72f3795

ntelkedzhiev added 3 commits September 18, 2018 14:06

Merge branch 'master' of https://github.com/relay-tools/react-relay-n…

40c6fe6

…etwork-modern into retry-handle-network-error

Fix babel to work with babel.config.js

13111c9

Fix retrying on internal errors

753525a

review: add test, update README, small other fixes

86ca508

nodkz added 3 commits September 19, 2018 19:05

refactor: retry middleware

a219675

feat: create and expose errors RRNLError

9a7b421

chore: update dependencies

439e1f1

nodkz added 6 commits September 19, 2018 19:49

ci: update node from v9 to 10

e46b7cd

feat: add req object to beforeRetry callback

f937d42

docs: fix attempt

1711c3f

feat: avoid unhandled promise rejection error

02d9dbe

feat: enable executeWithEvents method in RelayNetworkLayer class

b820f9d

test: fix unregular fails in retry tests

83d41bc

nodkz merged commit 47806aa into relay-tools:master Sep 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle network error in retry function and add a callback #37

fix: handle network error in retry function and add a callback #37

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

nodkz commented Sep 17, 2018

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

nodkz commented Sep 17, 2018

nodkz commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 18, 2018 •

edited

Loading

nodkz commented Sep 18, 2018

nodkz commented Sep 18, 2018

nodkz commented Sep 18, 2018

ntelkedzhiev commented Sep 19, 2018 •

edited

Loading

nodkz commented Sep 19, 2018

nodkz commented Sep 19, 2018

nodkz commented Sep 19, 2018

nodkz commented Sep 19, 2018

ntelkedzhiev commented Sep 20, 2018

fix: handle network error in retry function and add a callback #37

fix: handle network error in retry function and add a callback #37

Conversation

ntelkedzhiev commented Sep 17, 2018 • edited Loading

ntelkedzhiev commented Sep 17, 2018 • edited Loading

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 • edited Loading

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 • edited Loading

nodkz commented Sep 17, 2018

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 • edited Loading

nodkz commented Sep 17, 2018

nodkz commented Sep 17, 2018 • edited Loading

ntelkedzhiev commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018 • edited Loading

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 17, 2018

nodkz commented Sep 17, 2018

ntelkedzhiev commented Sep 18, 2018 • edited Loading

nodkz commented Sep 18, 2018

nodkz commented Sep 18, 2018

nodkz commented Sep 18, 2018

ntelkedzhiev commented Sep 19, 2018 • edited Loading

nodkz commented Sep 19, 2018

nodkz commented Sep 19, 2018

nodkz commented Sep 19, 2018

nodkz commented Sep 19, 2018

ntelkedzhiev commented Sep 20, 2018

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

nodkz commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 17, 2018 •

edited

Loading

ntelkedzhiev commented Sep 18, 2018 •

edited

Loading

ntelkedzhiev commented Sep 19, 2018 •

edited

Loading