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

chore: update main with outstanding fixes on v16.x.x branch #4171

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

yaacovCR
Copy link
Contributor

@JoviDeCroock @robrichard @benjie

In this PR, the outstanding v16.x.x changes highlighted by the work in #4165 have been cherry-picked and include the following PRs:

#3686 Workaround for codesandbox having bug with TS enums
#3923 instanceOf: workaround bundler issue with process.env #3923
#4157 Add GraphQLConf 2024 banner #4157

Why were these 3 PRs not developed on main like all the authors? Well, for the first two, it was thought at the time of the original work that a different strategy would be pursued in v17. See the individual links for more information, but, in short, for #3686 it was thought that in v17 TS enums would be entirely deprecated and for #3923 there were a number of proposals, including removing instanceof checks entirely. For #4157, the README change is only meant for the current 16.x.x default branch so that the GraphQL Conf banner can be shown on the publicly seen README and would be most appropriate on main if that became the default branch, although it would not cause any direct harm.

  1. In terms of code changes, this PR exactly matches the alternative merge commit strategy to align 16.x.x with main in Backport 16.x.x into main #4165. See: https://github.com/graphql/graphql-js/compare/backport-16.x.x..yaacovcr:cherry-pick
  2. If this PR is merged to main using the "rebase" strategy instead of the "squash" strategy, (A) we will get 3 separate commits on main with just like every other (recent?) change on main, (B), the original author information and the link to the original PR will be retained, (C) the git blame will point to the commit on main with the link to the original PR.
  3. If we choose the "rebase" strategy as I am suggesting, The gen-changelog script will list only this PR, i.e. a chore, so that readers of the changelog will not be aware that only v17 alpha 8 will have the exact same instanceof changes as the latest v16 version. This could be felt to be appropriate: this PR "fixes" the v17 alpha to be up to date with the latest changes in v16.x.x. Alternatively, we could split this PR into 2 or 3 separate PRs, which is a very straightforward task that I would be happy to do, each of which could be merged with the rebase strategy.

A more divergent alternative strategy would be to merge neither this PR nor #4165 => and just directly make the above planned changes, i.e. deprecate the use of TS enums and deal with the desired instanceof behavior, and ignore the GraphQLConf banner, presuming that the conference will precede the v17 release. This was the original plan, but I see the benefit in doing something as the shared memory of promised work "to be done later" is a bit tricky, especially considering the change in the maintainer cohort and the long time frame of the v17 alpha work.

Finally, an additional option to consider is to merge this PR so that the git blame will be more "directed" and STILL do the merge commit a la #4165 after this PR if we want to -- if that provides some additional benefit.

Either way, I think for v17 we should definitely include a migration guide, which would definitely aid developers directly to understand what has changed between v17 and the latest version of v16. We have gotten feedback after the last major release that a migration guide would be quite helpful.

IvanGoncharov and others added 3 commits August 19, 2024 06:32
graphql#4022)

As surfaced in
[Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532)
this currently is a breaking change in the 16.x.x release line which is
preventing folks from upgrading towards a security fix. This PR should
result in a patch release on the 16 release line.

This change was originally introduced to support CFW and browser
environments which should still be supported with the `typeof` check CC
@n1ru4l

This also adds a check whether `.env` is present as in the DOM using
`id="process"` defines that as a global which we don't want to access on
accident. as shown in graphql#4017

Bundles also target `process.env.NODE_ENV` specifically which fails when
it replaces `globalThis.process.env.NODE_ENV` as this becomes
`globalThis."production"` which is invalid syntax.

Fixes graphql#3978
Fixes graphql#3918
Fixes graphql#3928
Fixes graphql#3758
Fixes graphql#3934

This purposefully does not account for
graphql#3925 as we can't address
this without breaking CF/plain browsers so the small byte-size increase
will be expected for bundled browser environments. As a middle ground we
did optimise the performance here. We can revisit this for v17.

Most bundlers will be able to tree-shake this with a little help, in
graphql#4075 (comment)
you can find a conclusion with a repo where we discuss a few.

- Next.JS by default replaces
[`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182)
you can add `typeof process` linearly
- Vite allows you to specify
[`config.define`](https://vitejs.dev/config/shared-options.html#define)
- ESBuild by default will replace `process.env.NODE_ENV` but does not
support replacing `typeof process`
- Rollup has a plugin for this
https://www.npmjs.com/package/@rollup/plugin-replace

Supersedes graphql#4021
Supersedes graphql#4019
Supersedes graphql#3927

> This now also adds a documentation page on how to remove all of these
@yaacovCR yaacovCR requested a review from a team as a code owner August 19, 2024 04:22
Copy link

netlify bot commented Aug 19, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit a5cc45c
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66c2c89a8a77160008a13d37
😎 Deploy Preview https://deploy-preview-4171--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.

Copy link

Hi @yaacovCR, 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

@yaacovCR yaacovCR changed the title chore: update main with all outstanding v16.x.x changes chore: update main with outstanding fixes on v16.x.x branch Aug 19, 2024
@yaacovCR yaacovCR added PR: bug fix 🐞 requires increase of "patch" version number PR: breaking change 💥 implementation requires increase of "major" version number and removed PR: bug fix 🐞 requires increase of "patch" version number labels Aug 19, 2024
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Sep 6, 2024

This was discussed at the latest GraphQL-JS WG meeting. The general consensus of the small group who attended was to take the approach in this PR and front-port the work done on 16.x.x to main. We can use git's magic to make sure that the individual who did the work keep their names on the commits in main (along with the front-porters). They also get credit in the release notes from their original work.

The disadvantage to #4165 is allowing a merge commit onto main, which will eventually again become the default branch, and a merge commit would make the history non-linear, and may make git blame more difficult to use.

Another advantage to #4165 is that then the main branch will never again report something like:

main is 310 commits ahead of, 29 commits behind 16.x.x.

A certain school of thought might prefer 16.x.x to never be "ahead" of main in any way.

The counterargument that consensus at the meeting felt was persuasive, was:

  1. This is working as designed, as 26/29 commits represent backport commits of work down on main, with the 3 remaining commits representing work done on 16.x.x that should be front-ported.
  2. A merge commit from 16.x.x into main will complicates our understanding of what actually happened, as the branch point of 16.x.x from main/17 again becomes difficult to spot and manage, and possibly creating extra work, with additional room for errors.
  3. Presuming that main will once again be the default branch, a merge commit now does not help unless we also commit to never backporting additional fixes to 16.x.x -- as 16.x.x will once again get ahead of main as additional backport commits are added -- or unless we continually do these merge commits, further interfering with the linear history.
  4. Or in other words, it can be argued that the problem as stated above is a non-problem, git is correctly reporting what happened.

@yaacovCR yaacovCR added PR: polish 💅 PR doesn't change public API or any observed behaviour and removed PR: breaking change 💥 implementation requires increase of "major" version number labels Sep 6, 2024
@yaacovCR yaacovCR merged commit 699c1fc into graphql:main Sep 6, 2024
21 checks passed
@yaacovCR yaacovCR deleted the cherry-pick branch September 6, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants