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

Upgrade to Apollo v4 #19173

Merged
merged 34 commits into from Jan 15, 2024
Merged

Upgrade to Apollo v4 #19173

merged 34 commits into from Jan 15, 2024

Conversation

innerdvations
Copy link
Contributor

@innerdvations innerdvations commented Jan 9, 2024

What does it do?

  • Upgrade from Apollo v3 to Apollo v4

Some significant changes:

  • Apollo v4 has many breaking changes such as server config options

  • upgrade from graphql v15 to v16 which has its own long list of breaking changes

  • upgrade from graphql-upload v13 to v15 (note: didn't use v16 because it's esm only)

  • Graphql Playground replaced with Apollo LandingLocal

    • Add helmet exceptions for it
    • Add service to check that landing page is enabled (so it can be detected outside the plugin, eg for core/security to configure helmet as needed)
  • added some test utility functionality so request agent can send specific headers globally or per-request

Follow-up in later PRs:

  • helmet exceptions on a per-route basis rather than for all special paths
  • investigate if the changes to Apollo v4 configuration and our current method of merging user config with default config have made anything 'impossible'; for example, context no longer exists in ApolloConfig and moved to the KoaMiddleware, so we may need to provide a mechanism for users to inject code and configuration there as well
  • investigate what codemods can mitigate the breaking changes in user code
  • rename all references to 'playground' since it's no longer accurate (Playground is EOL, only LandingLocalDefault is supported)
    • boolean 'playgroundAlways' will become strings productionLandingPage and landingPage with options 'LocalDefault' and 'ProductionDefault' (so that in the future we can then support other landing pages)
    • 'isPlaygroundEnabled' will then become 'landingPage' and return the current landing page being used
  • add documentation to strapi/documentation for at least the following breaking changes:
    • all the package upgrades themselves
    • the need for the x-apollo-operation-name header in multipart requests
    • the replacement of playground

Why is it needed?

Apollo v3 is already EOL, so Strapi v5 must support Apollo v4.

How to test it?

  • Going to /graphql should load the graphql landing page (which is now different than the old playground) and work (schemas loading, queries work, etc)
  • The Graphql API should still work the same as before
  • The graphql server within Strapi should still be accessible as before and user code should generally work, as long as it's not using Apollo v3 specific functionality
  • API tests should all still pass
  • try changing some plugins.js apollo configuration settings -- all the ones that weren't breaking changes should still work

Related issue(s)/PR(s)

#19202 - First follow-up on this to improve the helmet security config

BREAKING CHANGE: Update from 'apollo-server-koa' to '@apollo/server' and '@as-integrations/koa'
Copy link
Contributor

github-actions bot commented Jan 9, 2024

Size Change: +2.15 kB (0%)

Total Size: 1.32 MB

Filename Size Change
examples/getstarted/build/main.********.js 1.31 MB +2.08 kB (0%)
examples/getstarted/build/runtime~main.********.js 4.27 kB +76 B (+2%)
ℹ️ View Unchanged
Filename Size Change
examples/getstarted/build/bb4d0d527bdfb161bc5a.svg 2.33 kB 0 B
examples/getstarted/build/index.html 608 B -3 B (0%)

compressed-size-action

@innerdvations innerdvations added this to the 5.0.0 milestone Jan 9, 2024
@innerdvations innerdvations added source: plugin:graphql Source is plugin/graphql package pr: enhancement This PR adds or updates some part of the codebase or features labels Jan 9, 2024
@innerdvations innerdvations self-assigned this Jan 9, 2024
@joshuaellis
Copy link
Member

Few thoughts - is it worth abstracting the configs so we can upgrade this library without mass breaking changes?

Has there been any discussion on how we might tackle the ESM issue? This is one of a few packages we now have this issue with (prettier being the other off the top of my head). I'm concerned were gonna lock ourselves into a position where there are security issues but we can't fix them.

@innerdvations
Copy link
Contributor Author

innerdvations commented Jan 10, 2024

Few thoughts - is it worth abstracting the configs so we can upgrade this library without mass breaking changes?

I don't think so, most of the config is going to be specific to the graphql server used, and if we try to build in auto-fixes for breaking changes here it could just confuse people and create a headache for us to maintain. TBH, considering the amount of breaking changes they made here and what they plan for Apollo v5, Strapi v6 may shift to a different graphql server package altogether

We could potentially provide a legacy graphql plugin that would still support Apollo v3, seeing that they've extended EOL from Oct 2023 to Oct 2024. Or maybe we deprecate @strapi/graphql and create a new @strapi/graphql-apollo-v4 that strapi defaults to? I will bring it up with @marcoautiero

Has there been any discussion on how we might tackle the ESM issue? This is one of a few packages we now have this issue with (prettier being the other off the top of my head). I'm concerned were gonna lock ourselves into a position where there are security issues but we can't fix them.

This is definitely something we need to open a discussion on, doing this migration brought it even more to my attention. Let's sync on this asap!

@innerdvations innerdvations marked this pull request as ready for review January 11, 2024 11:33
@innerdvations innerdvations marked this pull request as draft January 11, 2024 11:33
}

// TODO: we shouldn't combine playground exceptions with documentation for all routes, we should first check the path and then return exceptions specific to that
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this and the TODO below are handled in #19202

@innerdvations innerdvations marked this pull request as ready for review January 11, 2024 15:27
Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

Nice work!

packages/core/core/src/middlewares/security.ts Outdated Show resolved Hide resolved
packages/plugins/graphql/server/src/bootstrap.ts Outdated Show resolved Hide resolved
packages/plugins/graphql/server/src/bootstrap.ts Outdated Show resolved Hide resolved
packages/plugins/graphql/package.json Outdated Show resolved Hide resolved
packages/plugins/graphql/package.json Show resolved Hide resolved
@@ -16,7 +16,7 @@ const getFirstLevelPath = map((path: string) => path.split('.')[0]);
const controller = {
async getNonLocalizedAttributes(ctx) {
const { user } = ctx.state;
const { model, id, locale } = ctx.request.body;
const { model, id, locale } = ctx.request.body as any; // TODO: add correct typings
Copy link
Member

Choose a reason for hiding this comment

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

I guess this shouldn't really be part (as any other change in i18n types) of the PR and will be fixed directly in main. Do we need to keep those changes anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert them and update from v5/main now that it's apparently fixed there

(Update)
Narrator: It was not fixed there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note on this: adding @types/koa-bodyparser to graphql caused it to start working throughout the monorepo, switching the type of body from 'any' to 'unknown'. So I had to "fix" CTB and i18n so that ts:test:back will pass. Functionally there's no difference, just allowing body to be treated as any as it was before this PR (and in the future, new code should treat it as unknown because it is)

@innerdvations innerdvations added the flag: 💥 Breaking change This PR contains breaking changes and should not be merged label Jan 12, 2024
@innerdvations innerdvations merged commit 13a2f8b into v5/main Jan 15, 2024
76 of 77 checks passed
@innerdvations innerdvations deleted the chore/upgrade-apollo-v4 branch January 15, 2024 13:55
@echoes-hq echoes-hq bot added the echoes/type: maintenance Upkeeping efforts, chores and catch-up corrective improvements that are not features nor bugs label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/type: maintenance Upkeeping efforts, chores and catch-up corrective improvements that are not features nor bugs flag: 💥 Breaking change This PR contains breaking changes and should not be merged pr: enhancement This PR adds or updates some part of the codebase or features source: plugin:graphql Source is plugin/graphql package
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants