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

Don't attempt to connect to devtools in non-browser environments #11971

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

jerelmiller
Copy link
Member

This was mostly already doing this. With #11969, we removed the check for typeof window to set the default value for devtools.enabled. The connectToDevtools function has some checks for typeof window as well, but currently it allows the setTimeout to run before it checks for typeof window.

This PR simplifies this by returning early if typeof window is undefined.

@jerelmiller jerelmiller requested a review from phryneas July 23, 2024 14:41
Copy link

changeset-bot bot commented Jul 23, 2024

🦋 Changeset detected

Latest commit: 272000f

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

Copy link
Contributor

github-actions bot commented Jul 23, 2024

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 39.22 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 47.88 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 45.45 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 34.31 KB (-0.02% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 32.2 KB (-0.01% 🔽)
import { ApolloProvider } from "dist/react/index.js" 1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.24 KB (0%)
import { useQuery } from "dist/react/index.js" 5.21 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.3 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.69 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.77 KB (0%)
import { useMutation } from "dist/react/index.js" 3.62 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.84 KB (0%)
import { useSubscription } from "dist/react/index.js" 4.41 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 3.46 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.49 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.15 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.99 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.64 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.07 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.72 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.39 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3.33 KB (0%)
import { useFragment } from "dist/react/index.js" 2.32 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.27 KB (0%)

/**
* Suggest installing the devtools for developers who don't have them
*/
if (!hasSuggestedDevtools && __DEV__) {
hasSuggestedDevtools = true;
setTimeout(() => {
if (
typeof window !== "undefined" &&
window.document &&
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this whole check (apart from the __APOLLO_DEVTOOLS_GLOBAL_HOOK_) around the setTimeout?

No need to schedule a timeout that won't do anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly!

Copy link
Member Author

Choose a reason for hiding this comment

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

if (
window.document &&
window.top === window.self &&
!(window as any).__APOLLO_DEVTOOLS_GLOBAL_HOOK__ &&
Copy link
Member

Choose a reason for hiding this comment

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

!(window as any).__APOLLO_DEVTOOLS_GLOBAL_HOOK__ &&

This line needs to be inside the setTimeout though as it should check after 10s - sorry for being confusing! 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep just realized that 😆. I'm trying to move too fast and do too many things at once!

Copy link
Member

Choose a reason for hiding this comment

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

🐌 🐌 🐌

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps this is the right combination: 272000f

Copy link

netlify bot commented Jul 23, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit cef8ebb
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/669fc11413b16a0008c9b8e3
😎 Deploy Preview https://deploy-preview-11971--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

Looks good to me

@github-actions github-actions bot added the auto-cleanup 🤖 label Jul 23, 2024
Copy link

netlify bot commented Jul 23, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 6a52c58
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/669fc1f32f1ed400086db8ea
😎 Deploy Preview https://deploy-preview-11971--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 23, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 272000f
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/669fc2a9ff55810008e96f9f
😎 Deploy Preview https://deploy-preview-11971--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jerelmiller jerelmiller merged commit ecf77f6 into main Jul 23, 2024
46 checks passed
@jerelmiller jerelmiller deleted the jerel/moar-devtools branch July 23, 2024 14:59
@github-actions github-actions bot mentioned this pull request Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants