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

feat: Switch from Yoga v2 to Apollo Server v4 #8727

Conversation

Moumouls
Copy link
Member

@Moumouls Moumouls commented Aug 31, 2023

Pull Request

Issue

Closes: #8728

Approach

  • Install graphql upload to support upload
  • Install GraphIQL from Yoga team since GraphIQL is standard, and the yoga team graphiql is fine
  • Take advantage of X-Parse-Application-Id as CSRF protection https://www.apollographql.com/docs/router/configuration/csrf/
  • apollo/server is more focused on NodeJS and have dedicated expressHandler, i hope it will be more stable in complex envs.
  • Now we have end to end stream system from client to bucket/storage system through GQL Upload API
  • Disable the cache control since it's not used at the moment and can slightly help for performance
  • Remove all it_only_node < 17, now upload works on major NodeJS version without issues
  • Added mime type package to auto add extension to filename without extension since it's the current behavior, without this we will have a breaking change

Performance tests:

  • Checked performance on ParseServerGraphQL test suite and complete test suite in my company with a lot of graphql requests, performance are close and quite the same.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: switch from Yoga v2 to Apollo Server v4 feat: Switch from Yoga v2 to Apollo Server v4 Aug 31, 2023
@parse-github-assistant
Copy link

parse-github-assistant bot commented Aug 31, 2023

Thanks for opening this pull request!

@Moumouls Moumouls requested a review from a team August 31, 2023 00:23
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (80b987d) 94.29% compared to head (7882801) 94.32%.

❗ Current head 7882801 differs from pull request most recent head c9fc40a. Consider uploading reports for the commit c9fc40a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8727      +/-   ##
==========================================
+ Coverage   94.29%   94.32%   +0.02%     
==========================================
  Files         186      186              
  Lines       14785    14809      +24     
==========================================
+ Hits        13941    13968      +27     
+ Misses        844      841       -3     
Files Coverage Δ
src/GraphQL/ParseGraphQLServer.js 94.73% <100.00%> (+0.85%) ⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js 97.78% <100.00%> (-0.01%) ⬇️
src/GraphQL/parseGraphQLUtils.js 92.00% <100.00%> (ø)
src/GraphQL/loaders/filesMutations.js 89.58% <88.88%> (+8.93%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtrezza
Copy link
Member

mtrezza commented Aug 31, 2023

I notice that this also requires Node 16, so we'll wait with merging info alpha until Nov.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Apart from the requirement of Node >= 16, does this have any breaking changes? It's fine if it does, since we can only release it with Parse Server 7 (2024) anyway.

@Moumouls
Copy link
Member Author

@mtrezza no breaking changen may be the error message of graphql upload, but developers should not rely on message.

Also i added mime type detection, since it's currently supported.

Yes @mtrezza it's not a problem to keep this PR for 2024 :)

package.json Outdated
@@ -48,6 +48,7 @@
"lodash": "4.17.21",
"lru-cache": "9.1.1",
"mime": "3.0.0",
"mime-types": "2.1.35",
Copy link
Member

@mtrezza mtrezza Sep 1, 2023

Choose a reason for hiding this comment

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

We already have mime, it seems that it's similar to mime-types. I think if possible we should decide for one of the two if they provide the same functionality.

@mtrezza
Copy link
Member

mtrezza commented Sep 1, 2023

Could you suggest an entry for the changelog?

The current title would only be a refactor, but from the PR comment it seems to include many more improvements, so it would be good to add them to the changelog. You don't have to write that into the PR title, I'll add the long version in the commit message and we keep the PR title short.

@mtrezza mtrezza added state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message block:major Needs to be resolved before next major release; remove label afterwards labels Sep 16, 2023
@parse-github-assistant
Copy link

The label block:major cannot be used here.

@parse-github-assistant parse-github-assistant bot removed the block:major Needs to be resolved before next major release; remove label afterwards label Sep 16, 2023
@mtrezza
Copy link
Member

mtrezza commented Sep 16, 2023

@Moumouls While this PR is still "recent" and to prepare it for merging in Nov, do you think you could address the mime dependency issue? Otherwise I'm afraid this PR will go stale and not get merged for a long time if no-one picks it up.

@Moumouls
Copy link
Member Author

Hi @mtrezza, we should definitely merge this one. It works really well.

What is the mime dep issue. How is it linked to this Pr ?

Just tell me when I need to refresh the branch, and I'll be happy to fix last details to get it merged !

@mtrezza
Copy link
Member

mtrezza commented Sep 16, 2023

See #8727 (comment).

@Moumouls
Copy link
Member Author

Nice catch @mtrezza , I'll take a look

@Moumouls
Copy link
Member Author

Moumouls commented Oct 29, 2023

Hi @mtrezza i replaced last yoga dep with the CDN version of Apollo Sandbox: https://www.apollographql.com/docs/graphos/explorer/sandbox

Also fixed your feedback to use current mime dep

My team use this branch in production since now 2 months everything works well.

@Moumouls Moumouls requested a review from a team October 29, 2023 11:44
@Moumouls Moumouls requested a review from mtrezza October 29, 2023 11:44
src/GraphQL/ParseGraphQLServer.js Show resolved Hide resolved
src/GraphQL/loaders/filesMutations.js Outdated Show resolved Hide resolved
// needed since we use graphql upload
requestHeaders: ['X-Parse-Application-Id'],
},
introspection: true,
Copy link

Choose a reason for hiding this comment

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

Introspection should probably be disabled by default for security reasons. Perhaps a config variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @kekami , you are right but it needs a dedicated PR, i created the FR: #8800

Co-authored-by: Manuel <[email protected]>
Signed-off-by: Antoine Cormouls <[email protected]>
@mtrezza
Copy link
Member

mtrezza commented Dec 25, 2023

@Moumouls We have begun merging the breaking change PRs for Parse Server 7. Could you please resolve the conflict in this PR so we can merge it?

@mtrezza
Copy link
Member

mtrezza commented Mar 2, 2024

Closing via #8959

@mtrezza mtrezza closed this Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch from Yoga v2 to Apollo V4
3 participants