Set immediate error callbacks

[dallonf]

This is an attempt to fix #1141 . Unfortunately I had a lot of trouble testing this - it's just hard to coerce Mocha to allow uncaught exceptions.

[calebmer]

Could we just use setTimeout instead of setImmediate? I’m going to look at the tests now and see why they are complaining.

[calebmer]

Could we just use setTimeout(() => { ... }, 0) here? Is there any advantage to using setImmediate?

[dallonf]

setImmediate was called out explicitly in this comment, not sure if there's a real advantage, or if it's enough to offset the loss in readability. I can drop it!

[calebmer]

Same here.

[calebmer]

It looks like TypeScript is failing in CI and not the actual tests. Could you try changing the Observer type to:

export interface Observer<T> {
  readonly next?: (value: T) => void;
  readonly error?: (error: Error) => void;
  readonly complete?: () => void;
}

I think I recall readonly fixing this error in the past for me.

[abhiaiyer91]

@calebmer tried applying your TS type fix but you still receive TS errors error TS2532: Object is possibly 'undefined'.?

[dallonf]

Went ahead and fixed the type errors. Now we're getting into the weird test stuff. I might be able to figure out how to fix some of them.

[calebmer]

@abhiaiyer91 the current commit should fix the errors 😊

[dallonf]

For the QueryManager result transformation transforms watchQuery() results test, it's failing because the response has actually been transformed three times: once in QueryManager.ts (line 443, when calling observer.next()), once in ObservableQuery.ts(line 198, in refetch after calling queryManager.fetchQuery()), and then again in the same place as the first time.

It was calling it three times before the changes in this PR, but the third call was made after the assertion, so the test passed. Now the timing has changed subtly. I have no idea what the desired behavior is here... it's possible there was already a bug here that the tests didn't catch until now.

[dallonf]

Similarly, the client forceFetch can temporarily be disabled with ssrForceFetchDelay test fails because it's very dependent on the subtle timing of the observer.

I wonder if it would be a less impactful solution to do something like this...

try {
  observer.next(/*...*/)
} catch( err) {
  // throw error outside the current flow
 setTimeout(() => { throw err; }, 0); 
}

It's a lot harder to read and understand what's going on, but it would probably break fewer tests. (And I have to wonder if other libraries and apps that depend on apollo-client depend on this sort of timing in subtle ways)

[calebmer]

I doubt this change actually breaks real apps. It’s unfortunate that it breaks a lot of tests 😣

[dallonf]

Yeah, had a much easier time with that approach: #1367. Let me know if I should close this one if you'd rather go with this approach and somehow fix all the tests.

[calebmer]

Yeah, #1367 looks really nice. I don’t think there is any real substantial difference between the two approaches. Since the tests are green let’s pursue that one 👍