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

Expose graphql-voyager into the workspace #17

Open
tlvenn opened this issue Mar 3, 2017 · 14 comments
Open

Expose graphql-voyager into the workspace #17

tlvenn opened this issue Mar 3, 2017 · 14 comments

Comments

@tlvenn
Copy link
Contributor

tlvenn commented Mar 3, 2017

@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 ?

@OlegIlyenko
Copy link
Owner

OlegIlyenko commented Mar 3, 2017

Yeah, I like the project. I think it is a great idea, we just need to wait for them to publish an NPM package graphql-kit/graphql-voyager#1

@tlvenn
Copy link
Contributor Author

tlvenn commented Mar 3, 2017

Oky great to hear !

@tlvenn
Copy link
Contributor Author

tlvenn commented Mar 10, 2017

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

@OlegIlyenko
Copy link
Owner

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).

@RomanHotsiy
Copy link

RomanHotsiy commented Mar 10, 2017

@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?

@OlegIlyenko
Copy link
Owner

OlegIlyenko commented Mar 10, 2017

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 :)

@RomanHotsiy
Copy link

RomanHotsiy commented Mar 10, 2017

@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
Copy link

RomanHotsiy commented Mar 24, 2017

@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 #18)

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

@tlvenn
Copy link
Contributor Author

tlvenn commented Mar 27, 2017

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
Copy link
Contributor Author

tlvenn commented Mar 27, 2017

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

@tlvenn
Copy link
Contributor Author

tlvenn commented Mar 27, 2017

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

@RomanHotsiy
Copy link

@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

@OlegIlyenko
Copy link
Owner

@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.

@RomanHotsiy
Copy link

RomanHotsiy commented Mar 29, 2017

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

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

No branches or pull requests

3 participants