graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

Backport 16.x.x into main

Open JoviDeCroock opened this issue 1 year ago • 10 comments

We could backport https://github.com/graphql/graphql-js/pull/3730 to v17 as it's reverted in 16.x.x CC @n1ru4l

This gets us to a linear history - in the future as we merge things into 16.x.x let's ensure we leverage merge commits

JoviDeCroock avatar Aug 15 '24 07:08 JoviDeCroock

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
Latest commit 58aca55dc4630907ed6182c92e355c6c183028ac
Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66bef346d6a68d000831f102
Deploy Preview https://deploy-preview-4165--compassionate-pike-271cb3.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.

netlify[bot] avatar Aug 15 '24 07:08 netlify[bot]

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

github-actions[bot] avatar Aug 15 '24 07:08 github-actions[bot]

It would be wise to look over https://github.com/graphql/graphql-js/blob/16.x.x/resources/gen-changelog.js and see if it supports merge commits or if support can be added easily. If not, it may make sense to move to something like changesets before this merge? Whatever we do, it should be at least as good as what we have already.

benjie avatar Aug 15 '24 08:08 benjie

@benjie you are right

Error: Commit 1f8ba95c662118452bd969c6b26ba4e9050c55da has no associated PR: 16.5.0

JoviDeCroock avatar Aug 15 '24 15:08 JoviDeCroock

It looks like the changes center around the following three areas:

  • Readme changes around graphql conf
  • Enum changes
  • instanceof changes and documentation for v16

Maybe we can:

  1. submit three separate pull requests for these three issues into v17
  2. run the change log script / release
  3. do the merge commit

in terms of going forward:

I think a solution is to just release v17 and then go back to having main be stable again ie continue to represent v17 for time being and then any v18 be a separate feature branch

Are we ready to release v17? I think so! All the changes around incremental delivery are only around refactoring with the actual format stable for quite some time.

yaacovCR avatar Aug 15 '24 20:08 yaacovCR

Two additional points:

This approach would mean that the merge commit itself has no substantial changes which I think would make the history more readily understandable. There would be a single merge commit without any actual changes only for the purpose of creating a non conflicting diff between v16.x.x branch and v17 / main — I am not 100% certain of the importance of creating this non-conflicting diff, but…

In terms of readiness to release v17 — I mean, incremental delivery is ready to release under the experimental tags not that it is ready for actual release.

yaacovCR avatar Aug 15 '24 21:08 yaacovCR

Are we ready to release v17? I think so! All the changes around incremental delivery are only around refactoring with the actual format stable for quite some time.

I think that is a working group discussion and shouldn't be done lightly, I know there are more things that needed attention from previous WG meetings. In particular the dual package hazard as well as removing instanceof.

This approach would mean that the merge commit itself has no substantial changes which I think would make the history more readily understandable. There would be a single merge commit without any actual changes only for the purpose of creating a non conflicting diff between v16.x.x branch and v17 / main — I am not 100% certain of the importance of creating this non-conflicting diff, but…

It makes the git blame accurate with backrefs to relevant PR's and explanations.

If not, it may make sense to move to something like changesets before this merge? Whatever we do, it should be at least as good as what we have already.

I think either we move to changesets or for this one release we do a manual changelog, I don't mind the latter and moving to changesets after the fact. My main concern is leaving this branch open for too long CC @benjie

Alternatively we add --first-parent to the revList retrieval.

JoviDeCroock avatar Aug 16 '24 06:08 JoviDeCroock

It makes the git blame accurate with backrefs to relevant PR's and explanations.

We could have that by frontporting the enum changes, instanceof changes (and the banner) to main without the merge commit? Meaning git blame would identify those lines. It was my understanding that merge commits make git blame more difficult, although I am less familiar with that workflow.

I didn’t mean to be cavalier btw about releasing v17 — apologies!. Just excited in that I believe the incremental delivery experiment should no longer be considered to be blocking.

yaacovCR avatar Aug 16 '24 07:08 yaacovCR

We could have that by frontporting the enum changes, instanceof changes (and the banner) to main without the merge commit? Meaning git blame would identify those lines. It was my understanding that merge commits make git blame more difficult, although I am less familiar with that workflow.

I understand but it would also take away the contributions from the folks who committed time to improving v16, from that point of view I personally prefer this approach over finding a way around our changelog currently lacking.

I didn’t mean to be cavalier btw about releasing v17 — apologies!. Just excited in that I believe the incremental delivery experiment should no longer be considered to be blocking.

No need to apologize at all, if I came across in any blame way I do apologize. I am very excited about incremental delivery but a new major is always a big effort for our consumers and I personally always fear leaving people behind (if we look at v14/v15, there's still a lot of folks left behind). My reasoning is more in that area, when we cut a major I would love for everyone to chime in and agree that we have everything we want in this major.

JoviDeCroock avatar Aug 16 '24 07:08 JoviDeCroock

@JoviDeCroock If you have time to take a look at #4171 and the discussion I included there, I am trying to show that I think that the very valid concerns around both linear history and author credit can both be addressed whilst still preserving the linear history on main.

yaacovCR avatar Aug 19 '24 04:08 yaacovCR

Should this be closed now?

benjie avatar Sep 17 '24 11:09 benjie