Skip to content
This repository has been archived by the owner on Jan 20, 2019. It is now read-only.

Add shell completion #23

Merged
merged 4 commits into from
Jan 4, 2016

Conversation

lazybensch
Copy link
Contributor

RFC & PR


## generation function

For a good user experience we don't want to figure out those suggestions on runtime or the completion feature would not be substantially faster then the `ember help` command. So there will be a __generation function__ that generates a JSON file once after ember-cli is installed and then during every `ember install some-addon` command to ensure that blueprints added by new addons are recognized aswell.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you describe the lifetime of this file? Where will it live etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

The cache key should be described in more detail, ember install is only run on the installers machine now on there co-workers machines.

It may be interesting to compute a runtime cache key that represents all add-ons and there versions?

My main concern here (as with all caching) is being confident in the invalidation strategy.

@stefanpenner
Copy link
Contributor

I am very +1 on this.

There is overlap with: #12 and ember-cli/ember-cli#4576 It seems both depend on a good internal structure to share and to share quickly.

cc @kellyselden

@lazybensch
Copy link
Contributor Author

How to cache

@stefanpenner ember install not running on every machine is a good point, haven't thought of it. Assuming that caching those commands inside a json file is the best idea, then we could run the generation function as a hook on npm install (without arguments) afaik npm lets you do this and that is, for every user, a necessary step to alter their available commands. (edit: i tested this and it works really good, but only if you invoke npm i without arguments so it would not be a silver bullet but we could use it in combination with ember install or something else)

Another idea would be to invalidate the file every time you execute an ember command. So while you typing the command, ember-cli is reading from the currently cached file and once you hit enter it spawns a process that will regenerate the file in the background while ember-cli is executing whatever you typed (the generation currently only takes about 1100ms)

Or not to cache

Regarding the necessity of a cache itself: as i said above, currently the generation takes around 1 second, that is long enough that we do not want to run it every time a user hits <tab>. BUT currently the generation function is way more powerful than it needs to be! It is totally agnostic of commands and blueprints. The way it works is that it has a fixed root object (the ember command) that has all the available commands as sub-commands. But from here on every command is deciding on its own what its subcommands are. So for example the generate command has a method cliCommands where it specifies that its own subcommands are "all blueprints", and the destroy method does this again. The help method also specifies custom cliCommands in listing all commands, including generate and destroy which in turn, again have to specifiy their cliCommands and parse all blueprints (that way you have autocompletion for ember help generate model).

The reason i chose this approach is because its very flexible. If you create a blueprint that takes a very powerful DSL as arguments, you could write your custom cliCommands method for that blueprint that provides shell completion for your DSL.

However if we sacrifice that power and just agree that no command has sub-commands, except for generate, destroy and help. We could hardcode that and speed up the generation quite a bit, maybe making it fast enough to compute on runtime. (blueprints would be gathered and parsed 1 time instead of 4, and commands 1 time instead of 2..)

PS: personally, i would rather find a good invalidation technique for a cache instead of that hard coded approach. the current logic will be a lot easier to maintain when ember-cli evolves + i might be emotionally attached to that code :D

PPS: Another idea on how to speed up the generation so that we could do it on runtime would be to force the developer to write a dedicated cliCommand object for every command or blueprint or whatever he or she wants a cli-command, those objects would be lightweight enough to load them fast. On the downside, this will add redundancy cause for every generate command you would also need a generate cli-command with only the meta data interesting for shell completion and for every model-test blueprint you would need a model-test cli-command etc. but on the upside this makes the shell completion logic more explicit in the code and we would keep all the flexibility of the feature we have right now. (after work i will put all of these thoughts into the RFC)

@lazybensch
Copy link
Contributor Author

small update: i was able to speed up the generation of all those sub commands from 1200ms to 40ms.

so there is no need for caching anymore we can just calculate them on the fly

@stefanpenner
Copy link
Contributor

small update: i was able to speed up the generation of all those sub commands from 1200ms to 40ms.

so there is no need for caching anymore we can just calculate them on the fly

rad!

@stefanpenner
Copy link
Contributor

@lazybensch @kellyselden I want to move ahead with both this and ember-cli/ember-cli#4576

There is overlap though, especially when it comes to the schema output.
Any recommendations on moving forward?

The only thing i know we should do, is to make should the JSON schema remains stable, so outside tooling and continue to rely on it.

@stefanpenner
Copy link
Contributor

after chatting with @kellyselden we are going to merge #4576 ultimately we need to break the tie somewhere. The goal will be to massage the PR implementing this RFC in.

Anyways, I suspect both solutions will benefit from each other. Great work guys!

@kellyselden
Copy link
Member

@lazybensch I will go through your code and look for places you can use my stuff, and vice versa.

@lazybensch
Copy link
Contributor Author

@stefanpenner i am totally fine with this! As I'm quite new to this project i'll take my time anyway.
@kellyselden you may wanna wait until tomorrow evening. i have refactored a lot locally but don't wanna push it atm. I also already had a look at you PR and feel comfortable to adjust my stuff once yours is finalized.

@kellyselden
Copy link
Member

@lazybensch Sounds good! My PR is merged now so it is as final as it's going to be.

stefanpenner added a commit that referenced this pull request Jan 4, 2016
@stefanpenner stefanpenner merged commit 44cef6d into ember-cli:master Jan 4, 2016
@stefanpenner
Copy link
Contributor

Im very much +1 on this, I will help get the PR to completion.

kategengler pushed a commit to kategengler/rfcs that referenced this pull request Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants