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

Backport 16.x.x into main #4165

Closed
wants to merge 30 commits into from
Closed

Backport 16.x.x into main #4165

wants to merge 30 commits into from

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Aug 15, 2024

We could backport #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

IvanGoncharov and others added 29 commits May 9, 2022 19:30
…tional arguments (#3645)

BACKPORT OF #3634

Deprecates the positional arguments to createSourceEventStream, to be removed in the next major version, in favor of named arguments.

Motivation:

1. aligns createSourceEventStream with the other exported entrypoints graphql, execute, and subscribe
2. allows simplification of mapSourceToResponse

suggested by @IvanGoncharov
#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 #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 #3978
Fixes #3918
Fixes #3928
Fixes #3758
Fixes #3934

This purposefully does not account for
#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
#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 #4021
Supersedes #4019
Supersedes #3927

> This now also adds a documentation page on how to remove all of these
…4124)

Co-authored-by: Erik Kessler <[email protected]>
Co-authored-by: Benedikt Franke <[email protected]>
Co-authored-by: Michael Hayes <[email protected]>
Co-authored-by: Mike Ciesielka <[email protected]>
Co-authored-by: Saihajpreet Singh <[email protected]>
@JoviDeCroock JoviDeCroock requested a review from a team as a code owner August 15, 2024 07:12
@JoviDeCroock JoviDeCroock requested review from benjie and removed request for a team August 15, 2024 07:12
Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 58aca55
🔍 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.

Copy link

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

@JoviDeCroock JoviDeCroock force-pushed the backport-16.x.x branch 2 times, most recently from b95bc51 to 7753133 Compare August 15, 2024 07:17
@JoviDeCroock JoviDeCroock changed the title Backport 16.x.x Backport 16.x.x into main Aug 15, 2024
@JoviDeCroock JoviDeCroock requested a review from a team August 15, 2024 07:28
@benjie
Copy link
Member

benjie commented Aug 15, 2024

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.

.github/workflows/pull_request.yml Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Aug 15, 2024

@benjie you are right

Error: Commit 1f8ba95c662118452bd969c6b26ba4e9050c55da has no associated PR: 16.5.0

@yaacovCR
Copy link
Contributor

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
Copy link
Contributor

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.

@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Aug 16, 2024

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.

@yaacovCR
Copy link
Contributor

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.

@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Aug 16, 2024

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.

@yaacovCR
Copy link
Contributor

@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.

@benjie
Copy link
Member

benjie commented Sep 17, 2024

Should this be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.