apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.34k stars 2.66k forks source link

Add back "fix: skip PR creation if no prerelease changesets exist"" #11941

Closed jerelmiller closed 2 months ago

jerelmiller commented 2 months ago

Reverts apollographql/apollo-client#11939

Adds back the logic there before. I believe the check for no changesets is preventing any new prerelease PRs from getting created if we haven't yet done a prerelease. We'll need to figure out how best to work around this while ensuring we aren't opening prerelease PRs for changes merged to main (which I believe is what this change fixed).

changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: 3a027ba390be8330e4bd552f6c094d8b8e83839d

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

github-actions[bot] commented 2 months ago

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 38.68 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 47.48 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 45.05 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 34.22 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 32.08 KB (0%)
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.32 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.39 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.6 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.67 KB (0%)
import { useMutation } from "dist/react/index.js" 3.59 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.81 KB (0%)
import { useSubscription } from "dist/react/index.js" 3.23 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.43 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.47 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.12 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.98 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.63 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.04 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.69 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.35 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3.3 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%)
alessbell commented 2 months ago

I'm still noodling on the best way to prevent the issue this was attempting to fix, but I'm convinced this is not the way :D So I think we can close this PR for now, or re-revert it :D

The scenario that happened in March:

In practice, we should cut prereleases after we've released any pending patch versions from main because it vastly simplifies things. We could automate this by fetching the list of changesets on main and x-referencing it with the changesets on the prerelease branch, and only doing a changeset version if there are pending changesets other than unreleased-from-main ones. If all that sounds good I can create a ticket :)

alessbell commented 2 months ago

And we don't need to make any changes right now - with this change back in place, it only blocks the first Version Packages (alpha) when the pre.json changesets array is initially empty, as you noted, so we have time to get a proper fix in.

jerelmiller commented 2 months ago

Oops pulled the trigger too soon 😂

In practice, we should cut prereleases after we've released any pending patch versions from main because it vastly simplifies things.

100% this. I'm ok with leaving this as a practice rather than trying to automate this for now.

There was an unreleased changeset on main and its slug got included in the array in pre.json

Does this happen only if we merge main into the release-x branch or does it also happen if there also happens to be changesets present on main. If the former, perhaps we should avoid merging main into the release-x branch until we've released those changes? I'm a bit naive on much of the internal workings here so forgive me if this is a dumb question.

Regardless, thanks for the context here! That is useful 🙂

alessbell commented 2 months ago

Does this happen only if we merge main into the release-x branch or does it also happen if there also happens to be changesets present on main. If the former, perhaps we should avoid merging main into the release-x branch until we've released those changes? I'm a bit naive on much of the internal workings here so forgive me if this is a dumb question.

Correct, it only happens if we merge main into release-x, so if we accidentally merge main, we can just wait until after the patch release, merge main again and then do the prerelease. Keeping this as a practice for now sounds good 👍

jerelmiller commented 2 months ago

Ok great! If we ever find it becoming a problem because we keep forgetting, we can always add automation later. Thanks so much!