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

Add VIM keybindings support #1031

Open
wants to merge 8 commits into
base: master
from

Conversation

@semanser
Copy link

semanser commented Nov 29, 2019

Issue: #909

This PR brings VIM key bindings support to the Graphiql.

Features:

  • Vim keybindings for Query editor
  • Vim keybindings for Variables editor
  • Toggle keybinding: Cmd-\ or Ctrl-\
  • Local storage persistance
  • Passing like an option to theGraphiQL component
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 29, 2019

Codecov Report

Merging #1031 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1031   +/-   ##
=======================================
  Coverage   43.66%   43.66%           
=======================================
  Files          66       66           
  Lines        3016     3016           
  Branches      654      654           
=======================================
  Hits         1317     1317           
  Misses       1426     1426           
  Partials      273      273

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51bdc0e...7c0294e. Read the comment docs.

@@ -69,6 +75,7 @@ export class QueryEditor extends React.Component {
require('codemirror/addon/dialog/dialog');
require('codemirror/addon/lint/lint');
require('codemirror/keymap/sublime');
require('codemirror/keymap/vim');

This comment has been minimized.

Copy link
@acao

acao Nov 29, 2019

Contributor

possibly do inline requires based on the chosen option? These files can be pretty large. pretty soon we will also replace all these requires with import().then()

This comment has been minimized.

Copy link
@semanser

semanser Nov 30, 2019

Author

For the toggling between vim/sublime modes, we need to have these files to be preloaded.

This comment has been minimized.

Copy link
@acao

acao Dec 14, 2019

Contributor

@semanser i think we're ready to accept this earlier than we though, but we really need to use import(), as we are switching all of this to use import() very soon. it will be pre-loaded for the minified bundle, and loaded async for more modern setups. it will work great i promise. its a great use case for contextually loading modules asynchronously, with sublime as the one thats loaded by default by the entrypoint bundle. then the VIM module makes no impact on the entrypoint bundle size, though it still impacts the min

@acao

This comment has been minimized.

Copy link
Contributor

acao commented Nov 29, 2019

awesome, thank you! can you write a few tests for your PR so codecov is happy

semanser added 2 commits Nov 30, 2019
@acao

This comment has been minimized.

Copy link
Contributor

acao commented Nov 30, 2019

ugh i hate to do this because you did a great job of implementing this, and i asked you to add tests.

actually, now that i think about it @semanser, this is going to have to wait for plugins in a couple months. the reason being is that we are also switching to monaco, and so we may end up defaulting to what is more of a vscode style keybinding. i already have a prototype somewhere of how we would provide plugins for vim/emacs modes (using monaco-vim and monaco-emacs) to demonstrate my proposal for the new graphiql plugin api.

also in any case where theres a new dependency required, we are trying to aim for pluginification, especially in this case, unless we can load the dependency asynchronously, which is how plugins will work.

for your purposes currently, is there a way you can just require codemirror-vim in a way that overwrites those bindings in a more minimal fashion? sounds messy but maybe it would work? just trying to keep things minimal so that we can lean on plugins for a multitude of reasons

@semanser

This comment has been minimized.

Copy link
Author

semanser commented Nov 30, 2019

ugh i hate to do this because you did a great job of implementing this, and i asked you to add tests.

That's totally fine, don't worry :)

for your purposes currently, is there a way you can just require codemirror-vim in a way that overwrites those bindings in a more minimal fashion?

What do you mean with that? Right now, I'm using the default codemirror vim keybindings (and I guess they should work fine). I guess for now, this implementation can work pretty well, and can be replaced later with plugins.

@acao

This comment has been minimized.

Copy link
Contributor

acao commented Nov 30, 2019

i guess what im trying to say is that this implementation is beyond the scope of graphiql currently, until we introduce plugins. so we will introduce vim support to graphiql natively as an option only via plugins, as it were now. despite how simple this PR is, it will add some size to the bundle that we are trying to reduce, so the idea is that almost all feature requests are being parlayed for plugins. this one would be an example of that.

@semanser

This comment has been minimized.

Copy link
Author

semanser commented Nov 30, 2019

@acao so, I guess adding more tests has no sense, because this PR is going to be rejected?

@acao

This comment has been minimized.

Copy link
Contributor

acao commented Nov 30, 2019

yeah, thats what i was trying to say earlier, my bad if i was unclear. @benjie would probably say the same thing at this point. i just got so excited about vim support and how nice of a PR it was, and then i started reviewing it like we could merge it now, my mistake!

@acao

This comment has been minimized.

Copy link
Contributor

acao commented Nov 30, 2019

https://github.com/acao/graphiql-1-poc-monaco/blob/master/src/plugins/graphiql-plugin-mode/ModePlugin.tsx here's some half-complete psuedocode as an example, I'll be introducing some more complete proposals soon.

@acao acao requested a review from benjie Dec 1, 2019
@acao

This comment has been minimized.

Copy link
Contributor

acao commented Dec 1, 2019

@benjie what do you think? maybe if we made the vim dependency an async bundle which we can do with webpack or CDN versions? that would still a context driven require import() or require.ensure(). i've implemented it before and it's unnoticable, especially since the syntax mode is loaded so early on. and besides, with the minified bundle, all the async chunks are concatenated into a single file anyways. so it would still drive up the size of the minified bundle a bit. but we just had a huge reduction so. I'm torn

@acao

This comment has been minimized.

Copy link
Contributor

acao commented Dec 1, 2019

vscode vim: 1.3M installs
vscode sublime text keymap: 830K installs

i mean, slightly apples to oranges in that the vim one isn't just a keybinding extension, but interesting to compare

@acao acao requested a review from orta Dec 1, 2019
@benjie

This comment has been minimized.

Copy link
Member

benjie commented Dec 10, 2019

Sorry for the delay; I've been incredibly busy and am currently spending a couple days catching up with 200+ GitHub notifications!

Some evaluation criteria:

👍 Passes this criteria

👎 Fails this criteria

Yet to be evaluated

  1. Bundle size: does it add significant weight to the compressed minified bundle? If less than ~2% I'm not too worried, but above that we should think more carefully
  2. 👍 Necessity: is it possible and desirable to instead allow people to implement this in their own GraphiQL by passing in a few props? For example, I can imagine passing in custom keybindings for toggling, passing in a callback that requires the relevant package, etc. This might require fewer changes but add more power (for example other codemirror plugins could be loaded?)
  3. 👍 Scope: does this expand the scope of GraphiQL beyond where we want it? I think given this is planned to be supported in the new GraphiQL it's not really expanding the scope
  4. Timing/backwards-compatibility: one issue of introducing this now and then later (soon) replacing it with a Monaco equivalent is that the plugins will likely work in different ways and support different parts of Vim, which may result in "X Vim action worked in version Y but doesn't work in version Z" which is something we couldn't address without working on the monaco plugin itself.

Q1 can be answered by comparing the size of the bundled result.

Q2 I'm on the fence with. I'm erring towards 👍 just adding it directly rather than adding a too-flexible props system that we're going to replace with a plugin system in the coming months.

Q3 I'm also on the fence with; since we're planning to replace codemirror it seems unwise to expand its scope currently, but also lots of people want Vim mode and we could add this easily now so people get to enjoy it months earlier. Erring towards 👍

Q4 Anyone have any idea of compatibility between the monaco and codemirror vim plugins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.