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

Enable strict mode in tsconfig and fix type errors #11200

Merged
merged 33 commits into from
Nov 10, 2023
Merged

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Sep 7, 2023

Closes #11199

Enables strict mode in our tsconfig.json and ensures our types/code are updated to handle the failures that arise from this change.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2023

🦋 Changeset detected

Latest commit: 94b07f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -1,14 +0,0 @@
export function filterInPlace<T>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This file isn't used anywhere, nor is it exported. Hooray for removing unused code 🎉

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.08 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.51 KB (-0.07% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.01 KB (-0.04% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.61 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.27 KB (+0.01% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.22 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.21 KB (0%)
import { useQuery } from "dist/react/index.js" 4.28 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.1 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.59 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.41 KB (0%)
import { useMutation } from "dist/react/index.js" 2.55 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.54 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 4.62 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.05 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.14 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.56 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 2.94 KB (0%)
import { useFragment } from "dist/react/index.js" 2.09 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.04 KB (0%)

// FragmentRegistry constructor directly. This reserves the constructor for
// future configuration of the FragmentRegistry.
constructor(...fragments: DocumentNode[]) {
this.resetCaches();
if (fragments.length) {
this.register.apply(this, fragments);
this.register(...fragments);
Copy link
Member

Choose a reason for hiding this comment

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

This transpiles to the old code, but if we switch to a newer target we save a few bytes.

@@ -778,20 +782,20 @@ class Stump extends Layer {
return this;
}

public merge() {
public merge(older: string | StoreObject, newer: string | StoreObject) {
Copy link
Member

Choose a reason for hiding this comment

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

This minifies even shorter than the old code.

Comment on lines +46 to +48
public register(...fragments: DocumentNode[]): this {
const definitions = new Map<string, FragmentDefinitionNode>();
arrayLikeForEach.call(arguments, (doc: DocumentNode) => {
fragments.forEach((doc: DocumentNode) => {
Copy link
Member

Choose a reason for hiding this comment

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

2384d2383
< var arrayLikeForEach = Array.prototype.forEach;
2398a2398,2401
>         var fragments = [];
>         for (var _i = 0; _i < arguments.length; _i++) {
>             fragments[_i] = arguments[_i];
>         }
2400c2403
<         arrayLikeForEach.call(arguments, function (doc) {
---
>         fragments.forEach(function (doc) {

This transpiles slightly longer, are we okay with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok with it.

Comment on lines +36 to +47
private data!: EntityStore;
private optimisticData!: EntityStore;

protected config: InMemoryCacheConfig;
private watches = new Set<Cache.WatchOptions>();
private addTypename: boolean;

private storeReader: StoreReader;
private storeWriter: StoreWriter;
private storeReader!: StoreReader;
private storeWriter!: StoreWriter;
private addTypenameTransform = new DocumentTransform(addTypenameToDocument);

private maybeBroadcastWatch: OptimisticWrapperFunction<
private maybeBroadcastWatch!: OptimisticWrapperFunction<
Copy link
Member

Choose a reason for hiding this comment

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

All of these are set in functions that are directly called from the constructor.

src/core/ApolloClient.ts Outdated Show resolved Hide resolved
@@ -113,7 +114,7 @@ export class QueryInfo {
// NetworkStatus.loading, but also possibly fetchMore, poll, refetch,
// or setVariables.
networkStatus?: NetworkStatus;
observableQuery?: ObservableQuery<any>;
observableQuery?: ObservableQuery<any, any>;
Copy link
Member

Choose a reason for hiding this comment

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

We are generally very inconsistent with the extends OperationVariables on TVariables, and I have to say I'm not too happy with OperationVariables, as it works with type-declared types, but not with interfaces (as those don't have an implicit index signature).
We might want to look into that - but not in this PR.

@@ -526,7 +530,7 @@ export class QueryManager<TStore> {
// either a SingleExecutionResult or the final ExecutionPatchResult,
// call the update function.
if (isFinalResult) {
update(cache, result, {
update(cache as TCache, result, {
Copy link
Member

Choose a reason for hiding this comment

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

the TCache generic argument here is essentially an any-cast. I'm not super happy about that, but on the other hand, realistically all the generic arguments on markMutationResult are any-casts

Comment on lines 5 to 6
export class HttpLink extends ApolloLink {
public requester: RequestHandler;
constructor(public options: HttpOptions = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

requester appears in the whole code base only here and it doesn't seem to be set or read anywhere - it's also not there at runtime. I guess this can be removed?

@@ -86,6 +86,7 @@ export function withMutation<

return (
<Mutation ignoreResults {...opts} mutation={document}>
{/* @ts-expect-error */}
Copy link
Member

Choose a reason for hiding this comment

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

not gonna fix types in the old HOCs...

Comment on lines +106 to +109
onError(
error,
clientOptions as MutationOptions<TData, OperationVariables>
);
Copy link
Member

Choose a reason for hiding this comment

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

We might want to look closer into these, but for now I'm just casting them around..

@phryneas phryneas requested a review from alessbell September 13, 2023 13:52
@phryneas
Copy link
Member

This is ready for review now.

@phryneas phryneas marked this pull request as ready for review September 13, 2023 13:52
@@ -146,7 +146,7 @@ describe("mergeDeep", function () {
});

it("supports custom reconciler functions", function () {
const merger = new DeepMerger((target, source, key) => {
const merger = new DeepMerger(function (target, source, key) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit puzzled that this test worked... maybe that test needs deeper recursion?

@@ -0,0 +1,2 @@
/** @internal */
export type TODO = any;
Copy link
Member

Choose a reason for hiding this comment

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

I added this type for any-casts we want to remove in the future (e.g. in a separate PR, like in this case). As it is imported everywhere, we can easily use TS to track all of it's usages down and eliminate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Love it!

@phryneas phryneas changed the base branch from main to pr/api-extractor September 14, 2023 11:47
@phryneas
Copy link
Member

The base for this PR is now the Api-Extractor branch, so this PR can be reviewed, but not be merged before that other branch is merged.

@@ -398,7 +398,7 @@ namespace Cache_2 {
// (undocumented)
interface BatchOptions<TCache extends ApolloCache<any>, TUpdateResult = void> {
// (undocumented)
onWatchUpdated?: (this: TCache, watch: Cache_2.WatchOptions, diff: Cache_2.DiffResult<any>, lastDiff: Cache_2.DiffResult<any> | undefined) => any;
onWatchUpdated?: (this: TCache, watch: Cache_2.WatchOptions, diff: Cache_2.DiffResult<any>, lastDiff?: Cache_2.DiffResult<any> | undefined) => any;
Copy link
Member

Choose a reason for hiding this comment

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

This file sums up all the relevant changes to our external api. (The main entrypoint, that is)

Base automatically changed from pr/api-extractor to main September 14, 2023 16:27
Copy link
Member

@phryneas phryneas 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 given this yet another full review, and I approve. @jerelmiller, please also give this another pass - and then let's get this in as soon as possible!

@jerelmiller jerelmiller changed the title [WIP] Enable strict mode and fix type errors Enable strict mode in tsconfig and fix type errors Nov 10, 2023
@jerelmiller jerelmiller merged commit ae5091a into main Nov 10, 2023
23 checks passed
@jerelmiller jerelmiller deleted the enable-strict-mode branch November 10, 2023 19:26
This was referenced Nov 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable strict setting in tsconfig.json
3 participants