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

suggestion: mock global fetch explicitly #11779

Merged
merged 9 commits into from
Apr 12, 2024

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Apr 11, 2024

@alessbell I'd love to hear your opinion on this:

It is just a thought that I just had when reviewing #11748 - should call createSchemaFetch always implicitly mock window.fetch?

That could cut down on the usability of the tool in some situations.

Instead, what I'd suggest would be a mockGlobal propery.

So different usages might look like

    using _fetch = createSchemaFetch(schema, {
      delay: { min: 10, max: maxDelay },
-    });
+    }).mockGlobal();

or

    const { restore } = createSchemaFetch(schema, {
      delay: { min: 10, max: maxDelay },
-    });
+    }).mockGlobal();

While

    const fetch = createSchemaFetch(schema, {
      delay: { min: 10, max: maxDelay },
    });

would just return a fetch function that can be passed around, but won't pollute global scope.

With that, using the plain object with using would end up with (at least?) a TS error:

    // The initializer of a using declaration must be either an object with a [Symbol.dispose]() method, or be null or undefined.
    using _fetch = createSchemaFetch(schema, {
      delay: { min: 10, max: maxDelay },
    });

But once https://github.com/rbuckton/proposal-using-enforcement lands (it's suggested for stage 1/2 in todays TC39 meeting), we could make use of [Symbol.enter] to implicitly call mockGlobal once it's used with using, so in the long term, the above code would work and mock global again.


An alternative to that could be an option like

    using _fetch = createSchemaFetch(schema, {
      delay: { min: 10, max: maxDelay },
      mockGlobal: true/false
    });

but I believe that might be a bit more clunky to handle.

Copy link

changeset-bot bot commented Apr 11, 2024

⚠️ No Changeset found

Latest commit: 6743b23

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@alessbell
Copy link
Contributor

@phryneas I really like this idea, and mockGlobal sounds like a nice intermediate step. I hadn't seen the strict enforcement of using proposal, I hope it advances :) If and when it makes it into a TypeScript release, we can come back and add auto-mocking support when used as a disposable 🚀

@alessbell alessbell marked this pull request as ready for review April 12, 2024 12:29
@alessbell alessbell changed the base branch from issue-11748-min-max-createSchemaFetch-threshold to release-3.10 April 12, 2024 13:03
Copy link
Contributor

github-actions bot commented Apr 12, 2024

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 38.61 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 46.49 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 44.04 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 34.16 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 32.04 KB (+0.01% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.24 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.22 KB (0%)
import { useQuery } from "dist/react/index.js" 5.27 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.35 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.51 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.58 KB (0%)
import { useMutation } from "dist/react/index.js" 3.51 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.73 KB (0%)
import { useSubscription } from "dist/react/index.js" 3.19 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.38 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.44 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.11 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.92 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.58 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.03 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.69 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.32 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3.26 KB (0%)
import { useFragment } from "dist/react/index.js" 2.29 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.24 KB (0%)

Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

Really like the pattern of calling a method on the returned object, e.g. mockGlobal, which then returns a disposable 🔥

@github-actions github-actions bot added the auto-cleanup 🤖 label Apr 12, 2024
Copy link

netlify bot commented Apr 12, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 3ced22a
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/6619315f82e1a100085640ef
😎 Deploy Preview https://deploy-preview-11779--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 Apr 12, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 6743b23
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/66193295f8997a0007072181
😎 Deploy Preview https://deploy-preview-11779--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.

@alessbell alessbell added this to the Schema-driven testing milestone Apr 12, 2024
@alessbell alessbell merged commit 243c538 into release-3.10 Apr 12, 2024
27 checks passed
@alessbell alessbell deleted the pr/createSchemaFetch/mock-global-explicitly branch April 12, 2024 13:38
alessbell added a commit that referenced this pull request Apr 12, 2024
alessbell added a commit that referenced this pull request Apr 12, 2024
    * feat: accept min and max delay in createSchemaFetch

    * chore: add snapshot of invariant error and add tests

    * chore: update api reports and .size-limits.json

    * suggestion: mock global `fetch` explicitly

    * chore: update tests

    * chore: extract api

    * chore: update .size-limits.json

    * Clean up Prettier, Size-limit, and Api-Extractor

    ---------

    Co-authored-by: Alessia Bellisario <github@bellisar.io>
    Co-authored-by: alessbell <alessbell@users.n
oreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants