search

rethinkdb / horizon

Horizon is a realtime, open-source backend for JavaScript apps.
MIT License
6.78k stars 349 forks source link

Allow "ejecting" a horizon app #769

Open deontologician opened 8 years ago

deontologician commented 8 years ago

In the next big release, we're adding plugins (#373) and middleware (#24) which includes moving to express for hz serve. Since our stack is becoming more modular, as people move from frontend only to writing a backend, it makes sense to provide something along the lines of create-react-app's eject functionality.

The idea is that you could do hz eject $PROJECT_NAME $OUTPUT_DIR and it would read your config and output Node.js code (very simple and readable) that calls out to various libraries you need on the backend.

It should emit code that:

sjmcdowall commented 8 years ago

Why express and not Koa? The (supposed) successor to express? I'm using it quite nicely (especially V2)

deontologician commented 8 years ago

Right now Express has a wider install base and a larger ecosystem. So, for instance, to use Passportjs ( #64 ) we'd need several extra shims to adapt it to koa. Since both koa and Hapi have shims for express stuff, it's a better target.

That being said, once you eject, you are free to swap out express and switch to koa. I would say if you have an strong opinion on koa vs. express, you're probably not going to be ejecting anyway, rather writing a node app from the start.

sjmcdowall commented 8 years ago

@deontologician -- I am not sure I agree with your reasoning. Has anyone done any sort of heads-to-heads analysis of Express Vs. Koa? I mean .. the passportjs example .. how is it more than just a simple use of https://github.com/rkusa/koa-passport/tree/v2.x ??

I think there is a quite a large set of middleware written for Koa .. and it's pretty easy to write new middleware if needed..

And the speed differential -- at least the majority of speed tests show Koa to be more performant (some by quite a bit). Of course, anything outperforms Hapi. :)

Still the elegance of writing async / await type handlers is sooo clean ... (IMHO)

Anyway -- wanting to see Koa get a fair chance.. LOL

deontologician commented 8 years ago

We'll keep it in mind. Node also doesn't have async/await yet, so it may be a bit too early to jump to Koa 2.0

sjmcdowall commented 8 years ago

Node doesn't have import / export either -- so since we're going to use babel it's just a matter of adding : 

    "babel-plugin-syntax-async-functions": "^6.13.0",

to dependencies and 

  },
  "babel": {
    "plugins": [

      "transform-async-to-generator",

Then viola!

It's just that as a new project -- it would be a shame to look backward to "old" technology and not embrace what will be the future ...

deontologician commented 8 years ago

https://github.com/benjamn/recast/blob/master/README.md could be nice for generating the code for this

marshall007 commented 8 years ago

@deontologician see my comment on using yeoman for this kind of stuff here. I also kind of think this issue is just a dupe of #583. If we're still considering providing generators for front-end code I don't see why we would generate backend/plugin code differently.

Furthermore, instead of a separate command, hz eject, I think this should just be part of hz init. Something like hz init <type> where type defaults to app (i.e. current behavior) and additional options include server and plugin.

deontologician commented 8 years ago

@marshall007 There's a couple of things going on here:

see my comment on using yeoman for this kind of stuff here. If we're still considering providing generators for front-end code I don't see why we would generate backend/plugin code differently.

You're right, we should use yeoman for this stuff, since they've spent a lot of time making the generator experience nice and we're now actually accumulating a good amount of generators.

I also kind of think this issue is just a dupe of #583.

I disagree, I think this particular use case warrants its own issue, since it needs to do some things differently than a from-scratch generator. It needs to read their toml-configs to find out what to generate. It needs to move several internally managed things to an external directory (for example, plugins will likely be installed in .hz/node_modules until you eject, and we'll automatically manage a .hz/package.json for you etc). The idea is you can go from an existing production horizon frontend-only app, to a node app that does the equivalent functionality. It's possible to do that in yeoman, but it's not 100% obvious from #583's discussion exactly what to do to add ejecting.

instead of a separate command, hz eject, I think this should just be part of hz init.

I think this is where we're thinking about this issue differently. The proposal here isn't just to create a skeleton for a plugin or a horizon node server, it's for transforming an existing frontend-only app into one that has a backend that's maintainable directly by the developer. From an implementation standpoint, they're both code generation, but the meaning to the developer is completely different.

This is a good idea though, we should provide skeleton generators for backend/plugin projects as well. If we move hz init to using yeoman, we could probably just make it an option the user can enter, rather than a command line flag.

marshall007 commented 8 years ago

@deontologician makes sense, I agree with everything you said.

One caveat being that I don't think ejecting the backend into the same repo is gonna be the common case. I think it's much more likely that once you get to that point, you'll want to manage backend stuff in a separate repo. Managing a project with multiple/nested package.json is notoriously painful and I'm not sure we wanna lead people down that road. I also doubt that many would find porting the existing hz serve functionality, CLI, and config parsing to their app very useful.

I think once you've made the decision to manage your own Horizon server, you want absolutely zero magic out-of-the box. In my view, hz eject should leave you with a plain ol' express server, nothing special.

I see the value in keeping things as similar as possible for the user after an hz eject, I'm just not convinced that's what users of this tool would be looking for.

deontologician commented 8 years ago

Right, I think we should let them specify where to eject to, the same directory seems bad for the reasons you mention, people like to keep front/backend code separate.

I think once you've made the decision to manage your own Horizon server, you want absolutely zero magic out-of-the box.

I can see where you're coming from, but I think the way we have no-magic is by importing the utility libraries explicitly, and having comments in the file saying what they do. For me, magic would be having a framework run my code for me, doing things behind the scenes that I couldn't see. Here, config parsing, all the plugins you're loading etc would all be explicitly defined as code in your js file, with explanations for what they do. We'll have to go option by option to decide how the code is emitted, but I think we can make something unintimidating.

I do think it's absolutely essential that nothing gets dropped when you're ejecting. The ideal behind Horizon has been a smooth upgrade path, and dropping things you "used to have" with hz serve seems like it would create a gap in that continuum for not much benefit.

marshall007 commented 8 years ago

@deontologician sometimes it seems like we're trying to solve these problems from both ends. hz init could just create .hz/server.js, a boilerplate express app preloaded with middleware and/or Horizon plugins that implement config parsing and other default behavior. We could generate a more formal, documented directory structure with things split out conventionally too, which might be nicer.

(This would require, among other things, that @horizon/server be implemented as a request handler rather than a full-blown server, but bear with me.)

I imagine the biggest argument against this would be that the generated code might need to be updated between major server versions. We could make this a bit more migration-friendly by saying something like /* Generated code: do not modify this block */. We could even hash the contents of such blocks and warn the user if we're attempting to overwrite code they've modified in the generated files.

The point is, hosted users would now be able to configure the server however they want, which makes the Horizon Cloud offering much more robust. The distinction just becomes whether you prefer to use the Horizon CLI/Cloud for convention-based configuration, deployment, hosting, etc or not.

With that in mind, all hz eject would need to do is migrate files out of .hz into a separate repo. It wouldn't have to do any code generation. This is also a less jarring experience for the user. Instead of being bombarded with new concepts, it's all the same files they're used to looking at, just migrated into a separate repo.

This also makes it a lot easier to migrate the other direction if you decide to go from self-host to Horizon Cloud. You could just hz init and copy the middleware configuration out of your existing express app into the appropriate .hz/* file(s).

marshall007 commented 8 years ago

@deontologician so tl;dr I'm saying why not have hz init generate all the code you're talking about hz eject do. Migrating back and forth would be easier if we just had users specify validators and stuff in convention-based JS files rather than as JS snippets embedded in TOML configs (that we may migrate to JS files later anyway).

deontologician commented 8 years ago

The reason I think it's better to have an explicit eject step is because the code we use in hz serve etc is going to be much more general than we'd want to give to the end user.

So internally, we'll likely have code that looks like

let plugins = configLoader.plugins.forEach(pluginName => pluginLoad(pluginName, config[pluginName].options));
let pluginRouter = new PluginRouter(plugins)

And I think the code ejected should be something like:

import { CollectionPlugin } from '@horizon-plugins/collection';
import { GraphQLPlugin } from '@horizon-plugins/graphql';
let plugins = [
  new CollectionPlugin(),
  new GraphQLPlugin({
    route: '/graphql',
    queries: /// some stuff,
  }),
];
let pluginRouter = new PluginRouter(plugins);

etc. The idea being since it isn't dynamic, we can make config and imports more explicit.

(this is totally fake code, I don't know the exact api, just for illustration)

marshall007 commented 8 years ago

@deontologician my thought is that the generated code would have something like .hz/plugins/index.js which would just contain:

import Config from '@horizon/config';
export default Config.loadDynamicPlugins();

At any point I can stop using the dynamic plugin loader and manually load/configure plugins however I want in that file. I can also use the dynamic loader in conjunction with any simple inline plugin definitions I choose to write. I think you were on to something when you said "nothing gets dropped when you're ejecting". I just think we can/should just make that more literally true.

marshall007 commented 8 years ago

I just don't see a compelling reason to lock down the hz serve workflow so tightly. All (or at least most) users are developers and I think laying all this out explicitly on the filesystem would make things more approachable, not less. If you were already the type of user that treated the .hz directory as a black-box then it wouldn't affect you anyway.

I'm just having trouble seeing why we should bake all this magic into the framework rather than just providing a set of conventions on how to structure your app with a CLI tool that understands them.

deontologician commented 8 years ago

There's an issue of surface area we're exposing as well. So you mentioned above the issue of changing the internal representation. If we keep the code of hz serve an internal detail, we're free to modify it. If we explicitly expose it and say "write whatever js you want here", we have less flexibility to make changes. If we keep the phases separate, we can maintain backwards compatibility even if we change the internal code a lot.

Maybe it's just me, but I think keeping code vs. config separate makes a lot of sense. You can modify declarative config programatically very easily. Adding user supplied js configs seems like it's going to complicate a bunch of things. Internally we keep having these kinds of discussions where we want to do something, and it's either easy because it's something stored in toml/json or it's ridiculously difficult because the thing is arbitrary js (validator functions and templates come to mind).

So for me, eject is a nice clean break between "horizon is doing it" and "I'm doing it". Not having to worry about a bunch of hybrid failure cases seems worth it to me.

marshall007 commented 8 years ago

There's an issue of surface area we're exposing as well.

As far as I can tell, the public surface area is roughly the same in both cases. As I hinted at, you'd probably want the generated code to produce something like Config.loadDynamicPlugins() because you should still get dynamic plugin loading behavior after you eject. That means the "configurator" stuff has to be part of the public API anyway. Moreover, the dynamic plugin loader just produces an array of plugins, the same API you'd expose to self-host/eject users. So you're stepping on toes by introducing breaking changes there either way.

Maybe it's just me, but I think keeping code vs. config separate makes a lot of sense. You can modify declarative config programatically very easily.

I'm not necessarily making an argument one way or another here. Though, I think there's something to be said for the flexibility of making that choice for yourself based on the complexity of the plugins you intend to use and/or existing config mechanisms in your infrastructure.

Not having to worry about a bunch of hybrid failure cases seems worth it to me.

The problem is once hz eject is implemented you're gonna be maintaining the "auto-configure" (CLI) code in addition to code that generates stand-alone code with the exact same behavior. All I'm saying is why can't we just write the generator code and have it work for both (the bonus "hybrid" option is merely a side-effect).