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

test(connectObservable): add test combining ErrorBoundary and Suspense #53

Closed
wants to merge 2 commits into from

Conversation

voliva
Copy link
Collaborator

@voliva voliva commented Jul 8, 2020

I've found this issue where if a stream throws an error before emitting any value, the error can't be caught in an ErrorBoundary.

I've made this test that describes the issue and fails with the current version. A codesandbox can be found here

For reference, react uses this sandbox to show how the error is caught also in data fetching. I had a quick look and I think that they have it working because the component throws an error if the promise rejected.

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #53 into main will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #53   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines          216       223    +7     
  Branches        23        24    +1     
=========================================
+ Hits           216       223    +7     
Impacted Files Coverage Δ
src/internal/react-enhancer.ts 100.00% <100.00%> (ø)

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 869be66...0d5852b. Read the comment docs.

Copy link
Collaborator Author

@voliva voliva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found a posible fix that seems to work, and doesn't break any of the other tests, but I can't say I'm proud of it. Again, happy to drop it in favour of a better fix.

It feels like React is not really polished on this end...

Plus... it really doesn't work for connectFactoryObservable

Comment on lines +63 to +65
if (hasError) {
throw error
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compared with the way that this is solved in React's example and within their read (our getValue() equivalent) they either return the value, throw the promise, or throw the error.

Comment on lines +73 to 77
.pipe(
takeUntil(race(onSubscribe, of(true).pipe(delay(60000)))),
catchError(() => of(true)),
)
.subscribe()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed because otherwise this subscription would receive an error without handling it - Raising an uncaught exception that jest would pick up as error even if (expect(true).toBe(true)).

Comment on lines +292 to +294
await componentAct(async () => {
await wait(0)
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ewww.

I couldn't find any doc on this, but by experimenting, it seems that when a thrown promise fails in Suspense:

  • It creates a new component instance. Yep, the reducer init function gets called, and if you have a setState(() => Math.random()) you'll get another value.
  • It retries rendering the component a couple of times.

It looks like the single componentAct can't pick this up. Chaining two toghether does fix it.

Do you smell that smelly smell?

@voliva
Copy link
Collaborator Author

voliva commented Jul 9, 2020

Closing this in favour of #54

@voliva voliva closed this Jul 9, 2020
@josepot josepot deleted the boundary-suspense branch July 9, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants