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

Improve type safety with noImplicitAny #1052

Merged
merged 104 commits into from
Sep 6, 2023
Merged

Improve type safety with noImplicitAny #1052

merged 104 commits into from
Sep 6, 2023

Conversation

jerelmiller
Copy link
Member

Partially addresses #1047

Enables the noImplicitAny TS rule and fixes all the TS errors as a result. In addition to the fixes, I provided a few improvements:

  • Added codegen for the local schema to improve type safety of client-only queries
  • Added ts-reset to fix some edge case weirdness
  • Some minor refactoring on Queries and Mutations views that avoids the need for 2 queries and instead uses a plain array.find(...) to get the selected query.

@jerelmiller jerelmiller mentioned this pull request Sep 5, 2023
@@ -29,30 +28,39 @@ export function getQueries(

queries.push({
document,
source: document?.loc?.source,
Copy link
Member Author

@jerelmiller jerelmiller Sep 5, 2023

Choose a reason for hiding this comment

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

This is not used when getting the query string, so I opted to remove this to avoid confusion on whether this is used.

@@ -64,7 +72,6 @@ export function getMutations(mutationsObj): QueryInfo[] {
return {
document: mutation,
variables,
source: mutation?.loc?.source,
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously this source was used to print the mutationString, but was inconsistent with the way queries were printed, which just used the print function from graphql on the document. For consistency, this was updated to do the same.

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.

Some small nitpicks, but also a big THANK YOU!

@@ -55,7 +68,7 @@ export function EntityView({ cacheId, data, searchResults, setCacheId }) {
`}
onClick={() => {
if (key === "__ref") {
setCacheId(value);
setCacheId(value as string);
Copy link
Member

Choose a reason for hiding this comment

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

May I suggest just converting it to a string if it's unknown?

Suggested change
setCacheId(value as string);
setCacheId(String(value));

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 should always be a Refererence object which would guarantee a string here, but not opposed either.

@@ -60,7 +60,7 @@ export const handleAuthenticationPostMessage = ({
"apolloStudioEmbeddedExplorerEncodedApiKey"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the full

    const partialEmbedApiKeysString = window.localStorage.getItem(
      "apolloStudioEmbeddedExplorerEncodedApiKey"
    );
    const partialEmbedApiKeys = partialEmbedApiKeysString
      ? (JSON.parse(partialEmbedApiKeysString) as Record<string, string>)
      : {};

block could be extracted into a helper function? It repeats three times.

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 noticed that too. I'll go back through in a separate PR and do a few more refactorings like this 🙂

src/extension/tab/hook.ts Outdated Show resolved Hide resolved
src/extension/tab/hook.ts Outdated Show resolved Hide resolved
src/Relay.ts Show resolved Hide resolved
@phryneas phryneas self-requested a review September 6, 2023 16:09
@jerelmiller jerelmiller merged commit fbf7aca into main Sep 6, 2023
@jerelmiller jerelmiller deleted the fix-anys branch September 6, 2023 16:23
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