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

create separate package for @graphql-helix/core #140

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mosch
Copy link
Contributor

@mosch mosch commented Nov 16, 2021

Hey there,

we have done our best to solve the package size issue.
This may not be complete, especially around the deno packages.

Looking for some early feedback on this one 🤞🏻 @n1ru4l @talentlessguy @danielrearden

@n1ru4l n1ru4l self-requested a review November 16, 2021 15:43
@n1ru4l n1ru4l self-assigned this Nov 16, 2021
@mosch
Copy link
Contributor Author

mosch commented Nov 29, 2021

@n1ru4l is there anything I can do to move this forward?

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 29, 2021

@mosch, I did not have a look yet, but will on Thursday. Can you rebase upon master until then?

# Conflicts:
#	packages/core/package.json
@vercel
Copy link

vercel bot commented Nov 30, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/graphql-helix/graphql-helix/DUwDk1qFoZ7hKWmQNRTdXUCwiW1s
✅ Preview: https://graphql-helix-git-fork-launchport-separate-packge-graphql-helix.vercel.app

@@ -1,5 +1,5 @@
import express, { RequestHandler } from "express";
import { getGraphQLParameters, processRequest, renderGraphiQL, sendResult, shouldRenderGraphiQL } from "../../lib";
import { getGraphQLParameters, processRequest, renderGraphiQL, sendResult, shouldRenderGraphiQL } from "../../../graphql-helix";
Copy link
Collaborator

@n1ru4l n1ru4l Dec 2, 2021

Choose a reason for hiding this comment

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

I think we should move this folder (test/implementations) to packages/graphql-helix.

@@ -1,8 +1,2 @@
export * from "./get-graphql-parameters.ts";
export * from "./process-request.ts";
export * from "@graphql-helix/core DENOIFY: DEPENDENCY UNMET (BUILTIN)";
Copy link
Collaborator

@n1ru4l n1ru4l Dec 2, 2021

Choose a reason for hiding this comment

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

This warning seems dangerous? DENOIFY: DEPENDENCY UNMET (BUILTIN)

I am actually not really aware of all the deno shenanigans, it might be worth adding a deno integration test suite in a separate PR first, so we can verify this PR does not break the behavior?

@n1ru4l
Copy link
Collaborator

n1ru4l commented Dec 2, 2021

@mosch Shouldn't the be also a @graphql-helix/graphiql package that exports the renderGraphiQL and the shouldRenderGraphiQL function? It is a bit inconsistent to me that shouldRenderGraphiQL is still part of @graphql-helix/core, while shouldRenderGraphiQL is part of graphql-helix.

Copy link
Collaborator

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

I added some comments

@saihaj saihaj mentioned this pull request Dec 2, 2021
@dan-lee
Copy link
Contributor

dan-lee commented Dec 21, 2021

Since @mosch probably will be out for a while, I might as well take this over.
I think your comment makes sense, and I'll happily move that function into @graphql-helix/graphiql. Is there anything else we're missing to move this forward by the other reviewers (besides a merge/rebase)?

Thanks ✌️

edit: (don't know why vercel tests fail)

@vercel
Copy link

vercel bot commented Dec 21, 2021

Deployment failed with the following error:

The most recent charge for your active payment method has failed. Please update it here: https://vercel.com/teams/graphql-helix/settings/billing.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jul 8, 2022

Hey @dan-lee, sorry for letting this slip so long. Do you mind rebasing and finishing this PR? Let's finish this and release it.

@dotansimha dotansimha removed their request for review August 16, 2022 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants