graphiql-workspace icon indicating copy to clipboard operation
graphiql-workspace copied to clipboard

Expose graphql-voyager into the workspace

Open tlvenn opened this issue 8 years ago • 14 comments

@OlegIlyenko what do you think about exposing https://github.com/APIs-guru/graphql-voyager into the workspace directly, maybe as a sticky tab on the right ?

tlvenn avatar Mar 03 '17 07:03 tlvenn

Yeah, I like the project. I think it is a great idea, we just need to wait for them to publish an NPM package https://github.com/APIs-guru/graphql-voyager/issues/1

OlegIlyenko avatar Mar 03 '17 09:03 OlegIlyenko

Oky great to hear !

tlvenn avatar Mar 03 '17 11:03 tlvenn

https://www.npmjs.com/package/graphql-voyager

tlvenn avatar Mar 10 '17 04:03 tlvenn

Just tried it. Looks like it will not work in a current state. From what I can tell, the NPM package contains only 2Mb webpack-compiled bundle and original TypeScript files (together with other files from git repo).

We can't really use typescript since it's not used in this graphiql-workspace (it uses es6 and compiled with babel). When I try to use it as described in the documentation, it explodes with following error:

Error: Cannot find module 'clipboard' from '[...]/graphiql-workspace/node_modules/graphql-voyager/dist'
    at [...]/graphiql-workspace/node_modules/resolve/lib/async.js:46:17
    at process ([...]/graphiql-workspace/node_modules/resolve/lib/async.js:173:43)
    at ondir ([...]/graphiql-workspace/node_modules/resolve/lib/async.js:188:17)
    at load ([...]/graphiql-workspace/node_modules/resolve/lib/async.js:69:43)
    at onex ([...]/graphiql-workspace/node_modules/resolve/lib/async.js:92:31)
    at [...]/graphiql-workspace/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:114:15)

I looked around a bit, but haven't found any mention of clipboard dependency anywhere.

It would be nice it npm module could provide es5-compiled artifacts instead of a full webpack bundle (similar to GraphiQL and graphiql-worksapce).

OlegIlyenko avatar Mar 10 '17 16:03 OlegIlyenko

@OlegIlyenko I have just released a new rc.1 version of voyager. The issue you're facing should be fixed there. I have also set up webpack+babel example (I tried to build with browserify and it worked for me too). Let me know if you have any other issues. Will be happy to help out.

Regarding exposing es5 compiled artifacts. I think this is not possible at the current stage as we heavily use postcss and other webpack loaders. In the nearest future I'm going to externalize dependencies as much as possible so it won't take 2MB.

What do you think?

RomanHotsiy avatar Mar 10 '17 18:03 RomanHotsiy

Sweet! Thanks a lot, @RomanGotsiy! I just checked it again, and it works 🙌 🎉

screenshot_031017_091121_pm

Though I'm not yet sure where to put it, so it's now right in the middle :D It works great in terms of interaction right after initial load! Some issues I noticed right away:

  • Looks like type-info-popover sticks out on the left :)
  • It probably will take a while to properly embed it. Looks like switching between tabs does not work at all. There is a bunch of error messages in the console. I assume that it has to do a lot with lifecycle and mounting/unmounting of the component.

My main concern is not the size of the artifact, but rather the fact that dist/voyager.js file contains not only the npm module code itself but also code for all of its dependencies due to webpack packaging. It's actually great for standalone usage, but a bit problematic if I would like to embed it in other application. If I understand it correctly, the contents of all shared dependencies are added twice. I indeed noticed the 2.5x increase in a size of browserified version of graphiql-workspace. uglifyjs was quite slow to begin with, but now it takes > 5 minutes to minify it :)

That said, I totally can understand that it is very early stage in development. So things are a bit rough around the ages, but it's completely fine at this stage. Just wanted to provide some (hopefully) useful feedback (just could not wait to experiment with embedding it, graphql-voyager is an awesome project!)

If you would like to look how I embedded it for this POC, you can check this branch:

https://github.com/OlegIlyenko/graphiql-workspace/compare/poc-voyager

@tlvenn maybe you also would be interested in experimenting with it :)

OlegIlyenko avatar Mar 10 '17 20:03 OlegIlyenko

@OlegIlyenko thanks for awesome feedback! 🙌

My main concern is not the size of the artifact, but rather the fact that dist/voyager.js file contains not only the npm module code itself but also code for all of its dependencies due to webpack packaging. It's actually great for standalone usage, but a bit problematic if I would like to embed it in other application. If I understand it correctly, the contents of all shared dependencies are added twice.

yes, that's why I'm going to pack only voyager files with webpack and externalize other dependencies as much as possible. Hope will finish it next week.

There is a bunch of error messages in the console. I assume that it has to do a lot with lifecycle and mounting/unmounting of the component.

Thanks for sharing your work on a branch. This will definitelly help me to catch all theese issues.

I will keep you updated about the progress here ✉️

RomanHotsiy avatar Mar 10 '17 21:03 RomanHotsiy

@OlegIlyenko hey.

Finally I found some time to fix those issues. I have released a new version and fixed a minor issue with integration into worksapce (opened PR https://github.com/OlegIlyenko/graphiql-workspace/pull/18)

I have also created lib bundle without majority of dependencies included. Now it is 692KB vs >2MB unminified.

RomanHotsiy avatar Mar 24 '17 09:03 RomanHotsiy

As far as UI integration goes @OlegIlyenko, I was thinking of using a sticky right tab with only the graphql-voyager icon in it.

tlvenn avatar Mar 27 '17 03:03 tlvenn

Oups overlooked your comment on tab integration, my bad. Let me know if I can assist in that area.

tlvenn avatar Mar 27 '17 03:03 tlvenn

@RomanGotsiy any chance some memoization on your side could help the re render process ?

tlvenn avatar Mar 27 '17 03:03 tlvenn

@tlvenn there is some kind of memoization within each separate Voyager instance. It is bound to schema and is used for view settings (enable/disable relay, change root). When schema is changed it is cleared. Would be great If can just use multiple instances of Voyager

RomanHotsiy avatar Mar 27 '17 07:03 RomanHotsiy

@RomanGotsiy thanks a lot! I will check it out as soon as I get some time 👍

Regarding UI integration. Maybe it would be ok at the beginning to make it fullscreen and activateable on-demand with big close button in the top right corner. Similar to what graph.cool did.

Based on what I saw in graph.cool integration and my own experiments, it re-fetches the schema every time it is shown. But I need to doublecheck it, maybe new version improves it or in my experiments I mount/unmount component every single time so the full lifecycle kicks in.

OlegIlyenko avatar Mar 28 '17 22:03 OlegIlyenko

it re-fetches the schema every time it is shown.

@OlegIlyenko fyi: you can provide either function or directly introspection as a javascript object via introspection property

RomanHotsiy avatar Mar 29 '17 07:03 RomanHotsiy