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

fix: GraphIQL offline install #8153

Closed
wants to merge 6 commits into from

Conversation

Moumouls
Copy link
Member

@Moumouls Moumouls commented Sep 6, 2022

New Pull Request Checklist

Issue Description

GraphQL Yoga GraphIQL used a always online strategy (using unpkg to load the graphql editor). But a change was introduced breaking current installation on yoga 2.6.
Now using graphql yoga in offline mode, using the package.

Related issue: NONE

Approach

Add yoga graphIQL package and also update yoga to last version.

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 6, 2022

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Related issue: #123 in the PR description, so I can recognize it.

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Base: 94.34% // Head: 87.40% // Decreases project coverage by -6.93% ⚠️

Coverage data is based on head (2a66820) compared to base (40810b4).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 2a66820 differs from pull request most recent head 86a0e56. Consider uploading reports for the commit 86a0e56 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8153      +/-   ##
==========================================
- Coverage   94.34%   87.40%   -6.94%     
==========================================
  Files         181      181              
  Lines       14388    14389       +1     
==========================================
- Hits        13574    12577     -997     
- Misses        814     1812     +998     
Impacted Files Coverage Δ
src/GraphQL/ParseGraphQLServer.js 94.00% <100.00%> (+0.12%) ⬆️
src/Adapters/Storage/Mongo/MongoCollection.js 6.97% <0.00%> (-90.70%) ⬇️
src/Adapters/Files/GridFSBucketAdapter.js 10.14% <0.00%> (-83.34%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 12.10% <0.00%> (-80.05%) ⬇️
src/Adapters/Cache/RedisCacheAdapter.js 17.39% <0.00%> (-73.92%) ⬇️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js 35.48% <0.00%> (-62.37%) ⬇️
src/Adapters/Storage/Mongo/MongoTransform.js 48.67% <0.00%> (-39.79%) ⬇️
src/GraphQL/loaders/filesMutations.js 41.93% <0.00%> (-38.71%) ⬇️
src/Routers/SessionsRouter.js 66.66% <0.00%> (-25.00%) ⬇️
src/LiveQuery/ParseCloudCodePublisher.js 84.21% <0.00%> (-15.79%) ⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza
Copy link
Member

mtrezza commented Sep 6, 2022

But a change was introduced breaking current installation on yoga 2.6.

From which version to which version was the change introduced?

Related issue: node

What does that mean; is this is Node.js issue?

@Moumouls
Copy link
Member Author

Moumouls commented Sep 6, 2022

@mtrezza sadly after a quick discussion with Laurin, the 2.6 version used a non-pinned version of GraphIQL via unpkg. So when they released a new version of GraphIQL Yoga on unpkg, all old versions are now broken... (like yoga 2.6). I agree it's a really bad way to fix the bug of unpinned version on unpkg, actually its breaking change on their side. On Parse side, now we will use the offline version (using the package). The damage is done now (maybe a too french expression), let's move on with this PR.

I'm not also a huge fan of "always online" setups. I didn't check that graphIQL from yoga node was loaded via unpkg.

Note: The parse dashboard is not affected by this issue. And users have workaround on their dev setup to use a local graphIQL ou playground app/webapp

@Moumouls
Copy link
Member Author

Moumouls commented Sep 6, 2022

Sorry

What does that mean; is this is Node.js issue?

This is a typo, now corrected

@mtrezza
Copy link
Member

mtrezza commented Sep 6, 2022

Do you think we should provide more context in the changelog regarding this fix for developers? Which Parse Server versions are affected?

@n1ru4l
Copy link

n1ru4l commented Sep 7, 2022

There are multiple things that contributed to this problem:

  • We did not pin the @graphql-yoga/graphiql package version within the import statement from unpkg.com
  • The unpkg CDN does not correctly follow the module resolution algorithm for determining the correct entry point based on the exports map defined within the package.json (nor is there a way to specify that a requested import is from within an esm context).
  • Before we released the @graphql-yoga/graphiql version (which lead to the older clients breaking that did not have the version pinned), the package was not fully ESM and CommonJS compatible on platforms (as it did not satisfy the resolution algorithm via the exports map)

We are aware that this is indeed a "breaking" change that occurred without really changing/releasing a new version for the @graphql-yoga/node and @graphql-yoga/common packages. As the "breaking" change was introduced via a non-node_modules dependency that was loaded from "unpkg.com".

The alternative to not breaking older versions of GraphQL Yoga would have been to NEVER again release a @graphql-yoga/graphiql package under the latest tag on npm, which in my opinion would just lead to a lot of confusion in the long run. Thus we decided to just push a new @graphql-yoga/graphiql version with the respective fixes for satisfying proper ESM support on all platforms (especially Node.js).

This "breaking" change occurred from @graphql-yoga/[email protected] -> `@graphql-yoga/[email protected]:

For people that can not upgrade, we recommend using patch-package for temporarily pinning the import to the last known working version, here is the patch for doing so:

patches/@graphql-yoga+common+2.6.0.patch

diff --git a/node_modules/@graphql-yoga/common/index.js b/node_modules/@graphql-yoga/common/index.js
index 84d596a..57cb814 100644
--- a/node_modules/@graphql-yoga/common/index.js
+++ b/node_modules/@graphql-yoga/common/index.js
@@ -365,7 +365,7 @@ function useHealthCheck(options) {
     };
 }
 
-const graphiqlHTML = "<!doctype html><html lang=en><meta charset=utf-8><title>__TITLE__</title><link href=https://www.graphql-yoga.com/favicon.ico rel=icon><link href=https://unpkg.com/@graphql-yoga/graphiql/dist/style.css rel=stylesheet><body class=no-focus-outline id=body><noscript>You need to enable JavaScript to run this app.</noscript><div id=root></div><script type=module>import{renderYogaGraphiQL as r}from\"https://unpkg.com/@graphql-yoga/graphiql\";r(root,__OPTS__);</script>";
+const graphiqlHTML = "<!doctype html><html lang=en><meta charset=utf-8><title>__TITLE__</title><link href=https://www.graphql-yoga.com/favicon.ico rel=icon><link href=https://unpkg.com/@graphql-yoga/[email protected]/dist/style.css rel=stylesheet><body class=no-focus-outline id=body><noscript>You need to enable JavaScript to run this app.</noscript><div id=root></div><script type=module>import{renderYogaGraphiQL as r}from\"https://unpkg.com/@graphql-yoga/[email protected]\";r(root,__OPTS__);</script>";
 
 function shouldRenderGraphiQL({ headers, method }) {
     var _a;
diff --git a/node_modules/@graphql-yoga/common/index.mjs b/node_modules/@graphql-yoga/common/index.mjs
index 7258b6d..26cd3fc 100644
--- a/node_modules/@graphql-yoga/common/index.mjs
+++ b/node_modules/@graphql-yoga/common/index.mjs
@@ -363,7 +363,7 @@ function useHealthCheck(options) {
     };
 }
 
-const graphiqlHTML = "<!doctype html><html lang=en><meta charset=utf-8><title>__TITLE__</title><link href=https://www.graphql-yoga.com/favicon.ico rel=icon><link href=https://unpkg.com/@graphql-yoga/graphiql/dist/style.css rel=stylesheet><body class=no-focus-outline id=body><noscript>You need to enable JavaScript to run this app.</noscript><div id=root></div><script type=module>import{renderYogaGraphiQL as r}from\"https://unpkg.com/@graphql-yoga/graphiql\";r(root,__OPTS__);</script>";
+const graphiqlHTML = "<!doctype html><html lang=en><meta charset=utf-8><title>__TITLE__</title><link href=https://www.graphql-yoga.com/favicon.ico rel=icon><link href=https://unpkg.com/@graphql-yoga/[email protected]/dist/style.css rel=stylesheet><body class=no-focus-outline id=body><noscript>You need to enable JavaScript to run this app.</noscript><div id=root></div><script type=module>import{renderYogaGraphiQL as r}from\"https://unpkg.com/@graphql-yoga/[email protected]\";r(root,__OPTS__);</script>";
 
 function shouldRenderGraphiQL({ headers, method }) {
     var _a;

A full example for applying such a patch can be found here: https://github.com/n1ru4l/graphql-yoga-graphiql-incident-patch-package

@mtrezza
Copy link
Member

mtrezza commented Sep 8, 2022

@Moumouls What does this PR mean for existing deployments? Is this a breaking change?

@Moumouls
Copy link
Member Author

Moumouls commented Sep 9, 2022

@mtrezza no breaking change here for current deployments. It's a fix.

But currently, all deployments using the "mountPlayground" option on parse server since the migration to yoga, hava a broken playground page (because of the version mismatch related to the issue mentionned by @n1ru4l )

@mtrezza
Copy link
Member

mtrezza commented Sep 9, 2022

Got it. The CI tells us that this PR requires to bump the node engine to >=14. We will only drop support for Node 12 with Parse Sever 6. So how should we proceed here? Is the Node 14 bump required in this PR?

@Moumouls Moumouls force-pushed the moumouls/fixGraphIQL branch from 0bff06a to 3980ef1 Compare October 16, 2022 12:21
@Moumouls
Copy link
Member Author

conflict fixed @mtrezza

@mtrezza
Copy link
Member

mtrezza commented Oct 16, 2022

CI fails with:

There are 1 dependencies that require a higher node engine version than the parent package (>=12.22.10 <19):

  • web-streams-polyfill requires at least node 14.0.0 (>= 14)

If this PR requires at least Node 14 we can only merge this into alpha in November, as preparation for Parse Server 6.

Also, could you suggest a more descriptive PR title, which will be used as the changelog entry? A developer should be able to understand what this fix means.

@Moumouls
Copy link
Member Author

Hi @mtrezza if i fix the conflicts are we good to go to merge this one for Parse V6 ?

@mtrezza
Copy link
Member

mtrezza commented Nov 15, 2022

I think so, I'll add this to the PS6 todo list

@mtrezza mtrezza mentioned this pull request Nov 15, 2022
31 tasks
@Urigo
Copy link

Urigo commented Nov 17, 2022

maybe worth rebasing on top of #8250 ?

# Conflicts:
#	package-lock.json
#	package.json
@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 fix: graphIQL offline install fix: GraphIQL offline install Dec 24, 2022
@Moumouls
Copy link
Member Author

@mtrezza i refreshed the PR, i think we can merge this one, we will see later for: #8250

@Moumouls
Copy link
Member Author

@mtrezza i'm not sure i tried to npm i with node 16 and node 18, with lockfile V2 it seems that 16 and 18 try to reorder the lockfile.

If you have an idea ?

@mtrezza
Copy link
Member

mtrezza commented Dec 24, 2022

Could you try to revert any changes in package-lock.json and package.json and then run:

npm i -E @graphql-yoga/[email protected]
npm i -E @graphql-yoga/[email protected]

It seems these are the dependencies you want to install. Whatever changes in the lock file happen because of that, we can only accept I guess. It shouldn't cause any issues.

@Moumouls
Copy link
Member Author

Moumouls commented Jan 5, 2023

okay it's better @mtrezza 🚀 thanks for your suggestions, i don't know what happened on my machine before

@mtrezza
Copy link
Member

mtrezza commented Jan 5, 2023

Great, could you suggest a changelog entry that is descriptive enough for developers to understand what this fixes?
You said this is not a breaking change since Parse Server 6 requires Node >=14 anyway, right?

@mtrezza
Copy link
Member

mtrezza commented Jan 14, 2023

@Moumouls Could you confirm that this isn't a breaking change, so we can release Parse Server 6 without this being merged?

@Moumouls
Copy link
Member Author

@mtrezza not a breaking change

@mtrezza
Copy link
Member

mtrezza commented Jan 14, 2023

Thanks for confirming, I've removed it from the tracking list.

@Moumouls
Copy link
Member Author

@mtrezza will be closed by a small adjustment in #8727

@Moumouls Moumouls closed this Oct 29, 2023
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.

4 participants