graphql / graphql-playground

🎮 GraphQL IDE for better development workflows (GraphQL Subscriptions, interactive docs & collaboration)
MIT License
8.75k stars 731 forks source link

Fix Electron build #1279

Closed vardhanapoorv closed 3 years ago

vardhanapoorv commented 3 years ago

Fixes #1273 .

Changes proposed in this pull request:

acao commented 3 years ago

thanks so much for working on this @vardhanapoorv ! hoping to finally check this out this weekend and give it a local run through

vardhanapoorv commented 3 years ago

That's great!

acao commented 3 years ago

wasn't able to get it running last weekend, hopefully I can get to it tonight!

vardhanapoorv commented 3 years ago

@acao Let me know if anymore changes are needed, when you get a chance to review it.

acao commented 3 years ago

@vardhanapoorv nice! it works in terms of fixing the initial schema choice screen

on yarn start, once I supply a remote endpoint though, i get a blank screen and this error image

vardhanapoorv commented 3 years ago

@acao I build it using npm run build and then tested release app with npm run release:mac before.

Screenshot 2021-04-04 at 11 29 48 AM
vardhanapoorv commented 3 years ago

@acao just tested with yarn start that also seems to be working for me. Let me know if I'm doing something wrong here.

Screenshot 2021-04-04 at 11 35 55 AM
acao commented 3 years ago

so, when I run yarn after cloning your PR, the yarn.lock file is very different. so this might be why its not working here or in CI. If you're able to submit a PR with a clean lockfile then we can move forward! by that i mean, when I run yarn locally after checking out your PR, there should be no change to yarn.lock.

to try and simulate the situation I'm having, try cloning this repo into a new folder, and running yarn, and trying the above commands, and you'll see the lockfile diff and other issues I'm talking about

what that indicates to me is that the node_modules you have locally are working, but whats in the yarn.lock and in the package.json files is not adding up

vardhanapoorv commented 3 years ago

@acao oh yeah right makes sense, thanks. This reminds me that I was facing some issue with global lockfile so I installed the dependencies for electron package from there which created a separate lockfile in that folder, which I'm thinking is wrong since no file existed before. I'm a bit confused with the build process of the packages in this repo. After your above comment, I did try a bit but I'm not completely clear if the process I'm following is correct or not. (And yes I face couple of similar errors as you saw). Can you tell me the steps which should be followed? Should I install dependencies from base folder or from the respective packages folder? Then should I build the packages separately from their respective folders(React build worked, but the html build failed for me)? Thanks for helping out.

linux-foundation-easycla[bot] commented 3 years ago

CLA Signed

The committers are authorized under a signed CLA.

vardhanapoorv commented 3 years ago

@acao please ignore the above questions, I actually managed to update it correctly I think. Take a look and let me know if you still face any errors.

acao commented 3 years ago

awesome! also, can you sign the CLA for GraphQL Foundation? this is a new thing now that playground is a GraphQL Foundation project, instead of Prisma's CLA bot.

vardhanapoorv commented 3 years ago

Done!

acao commented 3 years ago

@vardhanapoorv it works great for me locally! excellent work. not easy to deal with all these package resolutions!

vardhanapoorv commented 3 years ago

@vardhanapoorv it works great for me locally! excellent work. not easy to deal with all these package resolutions!

That's great, thanks for helping me out with my questions throughout this issue!

acao commented 3 years ago

@vardhanapoorv glad to help! the contribution effort is very welcome! if you'd like to help accelerate the follow up, here's what I had in mind for next:

vardhanapoorv commented 3 years ago

Absolutely, sounds good. I can tackle these next. As a separate PR right?

acao commented 3 years ago

@vardhanapoorv yes! separate PR for sure. I will merge this one now. Again, thanks so much!