Do not restore the promise chain on error, instead continue errors

[vjocw]

Hello,

I have an issue where I'm using the promise in one of my components and it is not able to catch the error because returning the error restores the promise chain to be handled by the next then.

For example inside the component:

  update = (data) => {
    this.props
      .updateData(data)
      .then(data => this.onSuccess(data))
      .catch(err => this.onError(err));
  };

The above code will never run this.onError(err) even if in the error case.

[KevinGrandon]

Thanks for the PR, I'll run it by the team. This seems like a reasonable change to me, but we would need to update the tests. Would you be able to update these tests?

[vjocw]

@KevinGrandon Hi, yes I will add a test or two and modify if necessary

[vjocw]

@KevinGrandon Just fixed the test. It was simpler than I thought..

[codecov]

Codecov Report

Merging #154 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage   97.01%   97.01%           
=======================================
  Files           1        1           
  Lines          67       67           
  Branches       14       14           
=======================================
  Hits           65       65           
  Misses          1        1           
  Partials        1        1

Impacted Files	Coverage Δ
src/index.js	97.01% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d29590a...b98db99. Read the comment docs.

[codecov]

Codecov Report

Merging #154 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage   97.01%   97.01%           
=======================================
  Files           1        1           
  Lines          67       67           
  Branches       14       14           
=======================================
  Hits           65       65           
  Misses          1        1           
  Partials        1        1

Impacted Files	Coverage Δ
src/index.js	97.01% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d29590a...b98db99. Read the comment docs.

[codecov]

Codecov Report

Merging #154 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage   97.01%   97.01%           
=======================================
  Files           1        1           
  Lines          67       67           
  Branches       14       14           
=======================================
  Hits           65       65           
  Misses          1        1           
  Partials        1        1

Impacted Files	Coverage Δ
src/index.js	97.01% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d29590a...b98db99. Read the comment docs.

[wentsul]

@KevinGrandon @lhorie This seems like a breaking change. Do we plan on updatingfusion-plugin-rpc-redux-react to catch exceptions by default? Otherwise all calls using withRPCRedux need to explicitly handle exceptions at the call site. Previously you only needed to worry about exceptions in the reducer.

I think ideally we'd default to discarding the exception since the common use case is to handle in the reducer and provide an option to propagate exceptions.

[lhorie]

I don't believe this is necessarily a breaking change, because 1) I don't believe resolving to an error was correct behavior and 2) this code could already throw before:

fusion-rpc-redux/src/index.js

Line 169 in d522c10

throw e;

[wentsul]

I guess it's technically not a breaking change in fusion-rpc-redux I believe it will cause a regression in fusion-plugin-rpc-redux-react due to its assumption that the failure will be handled in the redux reducer and doesn't have to be explicitly handled at the call site.

For example, the fusion docs recommend fetching data for a component as follows:

compose(
  withRPCRedux('fetchData'),
  connect({myData} => {myData}),
  prepare(props => {
    const {myData, fetchData} = props;
    if (myData) {
      return Promise.resolve();
    }
    return fetchData();
  })
)(MyComponent);

Previously, every time the sideEffect was run, an exception would emitted a failure action for fetchData that could be handled with createRPCReducer's failure case.

With this code change, if fetchData fails, we'll get an unhandled promise exception whenever the sideEffect is triggered in prepared.js.

Does that make sense?