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

refactor: upgrade @graphql-yoga/node from 2.6.0 to 2.6.1 #8043

Closed
wants to merge 2 commits into from

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jun 14, 2022

This PR was automatically created by Snyk using the credentials of a real user.


Snyk has created this PR to upgrade @graphql-yoga/node from 2.6.0 to 2.6.1.

merge advice
ℹ️ Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project.


  • The recommended version is 3 versions ahead of your current version.
  • The recommended version was released 22 days ago, on 2022-05-23.
Release notes
Package name: @graphql-yoga/node
  • 2.6.1 - 2022-05-23
  • 2.6.1-canary-bfb3b22.0 - 2022-05-23
  • 2.6.1-canary-65224d9.0 - 2022-05-23
  • 2.6.0 - 2022-05-12
from @graphql-yoga/node GitHub release notes
Commit messages
Package name: @graphql-yoga/node

Compare


Note: You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.

For more information:

🧐 View latest project report

🛠 Adjust upgrade PR settings

🔕 Ignore this dependency or unsubscribe from future upgrade PRs

@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 [Snyk] Upgrade @graphql-yoga/node from 2.6.0 to 2.6.1 refactor: upgrade @graphql-yoga/node from 2.6.0 to 2.6.1 Jun 14, 2022
@parse-github-assistant
Copy link

Thanks for opening this pull request!

  • ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #8043 (d218139) into alpha (42c9543) will decrease coverage by 10.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##            alpha    #8043       +/-   ##
===========================================
- Coverage   94.14%   84.14%   -10.01%     
===========================================
  Files         182      182               
  Lines       13691    13691               
===========================================
- Hits        12890    11520     -1370     
- Misses        801     2171     +1370     
Impacted Files Coverage Δ
...dapters/Storage/Postgres/PostgresStorageAdapter.js 2.42% <0.00%> (-93.05%) ⬇️
src/Adapters/Cache/RedisCacheAdapter.js 12.28% <0.00%> (-75.44%) ⬇️
src/Adapters/Storage/Postgres/PostgresClient.js 5.00% <0.00%> (-65.00%) ⬇️
src/GraphQL/loaders/filesMutations.js 37.93% <0.00%> (-41.38%) ⬇️
src/batch.js 77.19% <0.00%> (-17.55%) ⬇️
src/ParseServerRESTController.js 81.81% <0.00%> (-15.16%) ⬇️
src/GraphQL/transformers/mutation.js 87.73% <0.00%> (-9.44%) ⬇️
src/Routers/AggregateRouter.js 92.00% <0.00%> (-8.00%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 90.59% <0.00%> (-2.57%) ⬇️
src/Controllers/UserController.js 95.34% <0.00%> (-2.33%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42c9543...d218139. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Jun 14, 2022

Interesting that a patch version upgrade causes tests to fail. Either yoga didn't follow semver, or our tests are too strict.

@Moumouls
Copy link
Member

@mtrezza I sent a message to the Guild, they will investigate asap I think.

The failing test is related to an addition that I asked about max file upload size to keep same features as the old graphql-upload package. May be something is not correctlu covered on their side and our test suite has catched the error. I added this test during the Yoga gql switch ☺️

@n1ru4l
Copy link

n1ru4l commented Jun 24, 2022

Hey there, we decided that the status code 500 is incorrect as the user is responsible for sending a file that exceeds the limit. Thus 400 makes a lot more sense. We treated this change as a bug fix.

@Moumouls
Copy link
Member

thanks @n1ru4l i'll take a look to update the test

@mtrezza
Copy link
Member

mtrezza commented Jun 24, 2022

@n1ru4l Changing the error code is a breaking change; does that mean Yoga doesn't follow semver?

@n1ru4l
Copy link

n1ru4l commented Jun 24, 2022

Hey @mtrezza, Yoga follows semantic versioning. Sometimes it is hard to define what is considered a breaking change. From our perspective, the file upload limit was undocumented and treated as unstable. As we are adding tests for the feature we realized that we implemented it not according to the reference specification. https://github.com/jaydenseric/graphql-upload/blob/b1cdd2a913c5394b5ff5f89b28d79b949b0bdde5/processRequest.js#L175 Apparently we also messed up again as the status code should be 413. 😭
We treated this as a bug fix as we did not see wide adoption. Is this a deep concern for the users that are already using parse-server file uploads? If it is a big issue we can discuss reverting it and postponing this fix (or breaking change) for the next major version.
And shoutouts to your test suite, the coverage is really impressive!

@mtrezza
Copy link
Member

mtrezza commented Jun 24, 2022

In semver any breaking change (of an exposed API) requires a major increment, regardless of adoption rate. I'm asking for us to know whether we have to be careful in the future when upgrading Yoga if it follows a "romantic" interpretation of semver.

@n1ru4l
Copy link

n1ru4l commented Jun 24, 2022

Yoga if it follows a "romantic" interpretation of semver

Do you treat something like adding a property to an object as a breaking change? If someone uses Object.entries on that object or maps over a TypeScript type that could also be considered a breaking change. It is really hard to define a breaking change to the latest detail and there is no document that defines it clearly. What guidelines are you following to define what is and is not a breaking change?

We aim to follow strict semantic versioning and will take your concern into account regarding future "romantic breaking" changes.

@mtrezza
Copy link
Member

mtrezza commented Jun 25, 2022

If someone uses Object.entries on that object or maps over a TypeScript type that could also be considered a breaking change.

In projects where a high level of stability is required, these aspects are usually defined. You could say "the order / number of properties may change". I don't think we are at this level however when talking about a HTTP response code. If the response code is part of the public API, then strictly following semver means it's a breaking change.

@Moumouls
Copy link
Member

Hi @mtrezza , @n1ru4l, I think that a breaking change was maybe introduced.

But this is why I added a test during my development on yoga since I know that the size limit feature was introduced in yoga for Parse Server to meet our requirements for the migration.

Btw, @mtrezza @n1ru4l, anyone open to send the really quick PR to fix the status code ? ☺️

@mtrezza
Copy link
Member

mtrezza commented May 21, 2023

Closing due to conflicts; waiting for snyk to open a new PR.

@mtrezza mtrezza closed this May 21, 2023
@mtrezza mtrezza deleted the snyk-upgrade-3b049a23205e9cb21a5d38128312fec8 branch May 21, 2023 15:59
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.

5 participants