Apollo Client v4 compatibility

[jerelmiller]

I've got a large chunk of the code updated to use the new types and remove some features that no longer exist in Apollo Client 4.0 (many of them ObservableQuery methods).

This isn't 100% complete so I'm leaving this in draft now. If possible, I could use a little bit of help figuring out why some of the tests/types fail. At the very least, this should provide significant progress!

I will continue a bit more work on this, but wanted to get some initial work out there and start a discussion. Thanks!

Checklist:

• If this PR is a new feature, please reference a discussion where a consensus about the design
  was reached (not necessary for small changes)
• Make sure all the significant new logic is covered by tests

[jerelmiller]

FYI there were a couple changes to the types in @apollo/client that I'd like to propose we do in this library as well:

• The generic arguments are now consistently <TData, TVariables>. Apollo Client 3 had some cases where it was flipped and it was annoying.
• The default TData type is now unknown instead of any to provide better type safety

Thoughts on bringing those changes over here too?

[PowerKiKi]

Those all sounds like a great idea. I am all for type safety and consistency 👍

[jerelmiller]

Apollo Client now defaults to notifyOnNetworkStatusChange: true and with that change, ObservableQuery now emits an initial loading state rather than waiting for the result to finish loading. As such, there are now more calls to the next function.

This should replace the need for useInitialLoading (which I believe was already started in the base branch)

[PowerKiKi]

Yes, I think useInitialLoading should totally disappear from this code base. Instead users can leverage notifyOnNetworkStatusChange. And apollo-angular default behavior will follow apollo-client default behavior (so notifyOnNetworkStatusChange will be true).

[jerelmiller]

Drawing attention to this because it might be good to come up with a common pattern. dataState is new with 4.0 and it adds type narrowing to data. Here we can drop the ! assertion since dataState === 'complete' will set data to TData.

[PowerKiKi]

In similar situations, I created a helper that is a predicate to be used with rxjs's filter. Something roughly like so:

/**
 * Rxjs predicate to be used with rxjs `filter` operator to keep types flowing
 */
export function hasCompleteData(result:  ApolloQueryResult<Query>): result is TypeThatSaysThatDataReallyExists {
    return result.dataState === 'complete';
}

Then it can be used like this:

.valueChanges.pipe(
    filter(hasCompleteData),
    map(result => result.data.film), // result.data.film guaranteed to exists
);

With a bit more effort we could create a filterCompleteData() that can be used directly, so the even simpler:

.valueChanges.pipe(
    filterCompleteData(),
    map(result => result.data.film), // result.data.film guaranteed to exists
);

I feel that this could be something useful for a lot of apollo-client users, so I wonder if apollo-client should provide those sort of utilities, instead of apollo-angular ?

[jerelmiller]

@PowerKiKi I'm opening this up for review. It's by no means 100% finished in order to be releasable, but wanted to at least open this up so I can start the discussion. At the very least, this should give you some good progress on supporting 4.0!

[PowerKiKi]

I pushed a minor prettier fix.

Am I right to assume that the 4 failing unit tests are related to the new default of notifyOnNetworkStatusChange ?

[PowerKiKi]

I merged master into this branch in order to solve all e2e tests. Now only unit tests are failing. I won't do anything more until you have a chance to review my other comments and adjust code accordingly.