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 Jennifer support #766

Closed
wants to merge 3 commits into from

Conversation

imdrasil
Copy link

@imdrasil imdrasil commented Apr 21, 2018

WIP: this PR is not ready to be merged

ATM this PR is created to describe my current vision how to integrate Jennifer into Amber. After clarifying all aspects and details - tests suite will be added and related Jennifer release created.

Description of the Change

Add Jennifer and Sam support

Alternate Designs

The reason why Sam has been added as well is its usage by some Jennifer processes.

Benefits

  • Jennifer may be set as default ORM
  • Sam tasks may be invoked using amber utility
  • micrate and default jennifer migration dsl may be used depending on a project settings

Possible Drawbacks

@faustinoaq faustinoaq requested a review from a team April 21, 2018 12:59
shard.yml Outdated
@@ -70,6 +70,10 @@ dependencies:
github: amberframework/teeplate
version: ~> 0.5.0

inflector:
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have this shard some lines below 😅

Copy link
Author

Choose a reason for hiding this comment

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

missed this ). Will fix later

@damianham
Copy link
Contributor

Supporting many different ORMs is going to get complicated in recipes. Maybe the model folder in a recipe should have sub folders for each ORM. It would simplify the model template considerably.

I didn't look too deeply into Jennifer but support for scopes is nice. How does it present errors ?

@imdrasil
Copy link
Author

hi @damianham
about subfolders for models - totally agree with this approach. If this will be approved - will rework it with pleasure
about errors - if you are talking about validation errors, ATM they are stored as a string-based sentence, but they are generated using i18n translations (very similar how Rails ActiveModel does)

@imdrasil imdrasil force-pushed the add_jennifer_support branch 2 times, most recently from 4122cda to d9cefc1 Compare April 21, 2018 17:45
@damianham
Copy link
Contributor

damianham commented Apr 22, 2018

Hi @imdrasil

Actually I wouldn't bother reworking it because I don't think this is the right solution. I think the right solution to provide support for Jennifer is a recipe. It is a new feature that has just been merged into the main branch and over time I think it will solve many issues. There have been discussions about supporting plugins, and different build/packaging systems and even a suggestion to remove the built-in generators and replace them with a default recipe. Support for Jennifer could be provided by a recipe also and the core amber code will not need to be modified. However you have added jennifer to the model options and it gave me a decent idea which builds on the idea of extracting the built-in app generator and using a default recipe instead. What we could do is have a collection of default recipes for the various options and build the recipe name from the given options if no recipe option is given, e.g.

amber new myapp

create a new amber app from a recipe at default/granite
(equivalent to amber new myapp -r default/granite)

amber new myapp -d mysql -m jennifer -t ecr

create a new amber app from a recipe at default/jennifer (no need to differentiate between database engine and template language as yet)
(equivalent to amber new myapp -r default/jennifer)

@eliasjpr
Copy link
Contributor

@imdrasil this is awesome!

@damianham at the time there was no recipe feature, but you are right having this as a recipe is the ideal approach the good thing here is that I believe this entire PR can be the recipe with small to none of the changes, correct?

I will review this PR, it will take some time since is a big PR

@damianham
Copy link
Contributor

@eliasjpr yes that is correct this entire PR could be provided by a recipe. I have submitted a new PR to the amberframework/recipes repo which creates basic/granite and basic/crecto so to support Jennifer it is a case of copy the granite or crecto recipe to basic/jennifer and then amend accordingly.

@imdrasil
Copy link
Author

@damianham found it. So I will wait until it will be merged and rework this PR as a recipe. What do you think?

@damianham
Copy link
Contributor

@imdrasil yes I think that is the best way forward.

@damianham
Copy link
Contributor

damianham commented May 2, 2018

@imdrasil the basic recipes have been merged into the recipes repo now so you could start to implement a basic/jennifer ORM recipe

@robacarp robacarp added the pr:wip label May 2, 2018
@imdrasil
Copy link
Author

imdrasil commented May 2, 2018

Hm, as I can see recipes includes only markup. So I should move all markup there and leave all specific classes in this PR, am I right?

@damianham
Copy link
Contributor

I think that this PR will not be needed, everything to use jennifer in an app would be built in to the recipe. What you would probably do is start with a copy of basic/granite in basic/jennifer and modify the model and scaffold accordingly and if the Jennifer ORM model provides the same kind of interface to views as granite then you will probably not need to modify the scaffold views. I think the files to modify in the basic/jennifer recipe would probably be:
basic/jennifer/app/shard.yml.lqd
basic/jennifer/app/config/initializers/database.cr.lqd
basic/jennifer/model/src/models/{{name}}.cr.lqd
basic/jennifer/model/spec/models/{{name}}.cr.lqd
basic/jennifer/scaffold/controllers/src/controllers/{{name}}_controller.cr.lqd
basic/jennifer/scaffold/controllers/spec/controllers/{{name}}_controller_spec.cr.lqd

Then you can add a test in spec/recipes_spec.cr for your recipe.

If you want to go the extra mile you could also create a misc/modular_jennifer recipe and follow the directory layout of the misc/modular recipe which simply places the controller, models and views in the same folder. Adding a misc/modular_jennifer recipe after creating the basic/jennifer recipe should be as simple as copying the controller and model templates in the right place.

@imdrasil
Copy link
Author

imdrasil commented May 3, 2018

ok, will take a look closer later. Also I may be wrong, but it seems there is no way to use several recipes at the same time (e.g. jennifer and react).

@damianham
Copy link
Contributor

That is true unless there is a react/jennifer recipe. An app is created with a recipe and subsequent amber generate scaffold uses the scaffold template in the recipe that was used to generate the app. However in #749 I discuss an option whereby a recipe could depend on another recipe. The problem we will soon have a situation where we update the amber release and we than have to update the shard.yml file in all recipes, so recipe maintenance will become a real chore. By moving to a recipe dependency chain a recipe could be diffs from a base recipe. We are planning to implement before and after hooks and plugins which will give us more flexibility. The hooks will be for each kind of artifact being generated and stored within the recipe/hooks folder. We could also move the ORMs to plugins and install an ORM plugin during app generation. We may end up with everything in recipes and plugins and the existing built-in generators being completely removed. I just need to find some time to get around to implementing those features. For the moment if you can implement a basic/jennifer recipe that would be awesome.

@drujensen
Copy link
Member

drujensen commented May 3, 2018

@damianham @faustinoaq Are we going to remove the flags -m and -t and replace them with different recipes and plugins?

@drujensen
Copy link
Member

@damianham How do you upgrade the recipes on a project when a new version of Amber comes out?

@damianham
Copy link
Contributor

@drujensen I don't foresee any need to change the behaviour for the -t flag at the moment as it just selects whether to generate .ecr or .slang views and both are provided in the recipe template. We can retain the -m flag and internally convert that to an appropriate recipe by appending the given ORM name to 'basic/'. E.g -m granite (or no -m flag) would use basic/granite, -m crecto would use basic/crecto, -m jennifer would use basic/jennifer -m mongo basic/mongo etc. However I also like the idea of moving the ORM to a plugin so the -m would resolve to an ORM plugin and if we can move the ORM to a plugin then I am sure we would be able to move the views to a plugin so -t could support all of the template languages supported by crystal using the same principle.

One of the problems we will face with plugins is that with the default layout and the modular layout we have 2 directory layouts so plugins will have to support these 2 directory layouts. TBH I would be in favour of moving completely to the modular layout to avoid this issue. If you look at the react/preact_redux recipe I have everything related to a feature located in one folder i.e. model, controller, views, javascript and stylesheet. It is a good way to organise code. For example MEAN.JS organises an application this way.

As you pointed out in another PR or issue, maintenance of recipes is going to be a lot of work as new amber releases come out and so separating the generation of an amber app into a base app with the shards and config etc and then adding plugins according to the command line arguments may alleviate the burden somewhat.

I think @robocarp already mentioned that we would need to have a mechanism to auto update recipes as new releases come out. There is also an issue #733 to have a recipe cache per repository so maybe we can implement auto updating recipes along with that enhancement. Once your app is generated then you would have to update shards and config manually as you do now to use a newer version of amber or shards etc.

At the moment I am making decisions regarding recipes but I would really appreciate any suggestions and guidance from everyone about how you would like it to work. Also are we happy with the recipes organisation at the moment? Is it a good idea to have default at the root and everything else in a sub folder? Should modular recipe be at the root alongside default? Should modular be the default app template? Should we work towards a base app and then additional plugins as discussed above? There are many questions and everyone's feedback is appreciated. What would be even more appreciated is people chipping in and implementing some of these new features :-)

@drujensen
Copy link
Member

TBH I would be in favour of moving completely to the modular layout to avoid this issue.

I am against that. I am a fan of the Rails "separation of concern" layout and prefer it over the modular.

Should modular recipe be at the root alongside default? Should modular be the default app template?

No, No

I think @robocarp already mentioned that we would need to have a mechanism to auto update recipes as new releases come out.

Currently to update the templates, you update the shard.yml and run shards install. I am hopeful we don't add any more complexity than this.

To be honest, I am leaning toward recipes as being a bad idea altogether.

Rails popularity is because anyone could easily jump right in and they know exactly where everything is. It provides very simple scaffolding on purpose. It was never meant to be used as production code. The idea was to provide a recommended design pattern that everyone would be familiar with.

Amber out of the box generates the same simple structure that any Rails developer will feel right at home with. I do not want to lose that simplicity. It is also front-end agnostic on purpose since there are thousands of JS frameworks and we do not want to maintain solutions for all the different flavors.

It was brought up by @jwaldrip that we should lose the -m (and possibly -t) options because we should provide our recommended design pattern with our generators and simplify, not complicate, the out of the box solution. I am leaning more toward that approach.

What would be even more appreciated is people chipping in and implementing some of these new features

I wrote the original templates and I am happy with them.

Here are my concerns with recipes so far:

  • Code duplication
  • Zip files in git repositories
  • Recipes all living in the same git repo
  • Custom package management for recipes
  • Maintenance of Amber updates

Unfortunately, I don't have any time right now to help. I will have some time this summer. I have some idea's on how to address these concerns (mainly changing recipes into crystal libraries as shards) and will work on this when I get a chance.

@faustinoaq
Copy link
Contributor

@drujensen I understand your concerns, although, I think recipes is a great idea

Here are my concerns with recipes so far:

  • Code duplication: I think we can reuse code by using symlinks. Also We can manage this by just using amberframework/recipes for blessed recipes, so you still can use your own recipe url via recipes_source on your .amber.yml file. This can already allow you to have your own recipes stores one-recipe-one-repo or whatever your want.
  • Zip files in git repositories: I solved that, please see: Add automatic deploy recipes#14 Now .zip files are not stored on repo, and recipes are automatically tested and deployed on master, always pointing to dist tag.
  • Recipes all living in the same git repo: we already discussed that on Recipes feature review #750 so I think, we can reuse code here and avoid to have many repositories. So the amberframework/recipes repo is easier to maintain and review IMO.
  • Custom package management for recipes: This one is a bit confusing, recipes are not packages, they are just pre-created amber projects with custom config.
  • Maintenance of Amber updates: I think I already managed to do that as well, each recipe is a full amber project, so, you always can use the amber version that comes within the recipe, by example: amber new ambegram -r misc_react can be generated with amber v0.8.5 but the recipe itself can use v0.8.1 via bin/amber. Also I think we can offer important snapshots to save recipes across versions, by example if amber v0.8.0 is not compatible with v0.9.0, and I want to have 2 modular recipes for each version, then I can create a folder with misc/[email protected] with custom changes (even reusing code via symlinks) then using this amber new blog -r [email protected] (already possible with Add automatic deploy recipes#14)

Unfortunately, I don't have any time right now to help. I will have some time this summer. I have some idea's on how to address these concerns (mainly changing recipes into crystal libraries as shards) and will work on this when I get a chance.

I think recipes as shard would be an interesting idea (just like amber-saludo example), although, if you use shards then you can't generate a full project from scratch like recipes do, you will need to have a project first, well at least a shard.yml file and compile the generator again and again for each recipe you want to try.

I think recipes is a more elegant and easier to maintain approach.

Recipes are easier to test, maintain and deploy. One repo for blessed recipes is the way to go IMHO. 😅

WDYT?

@drujensen
Copy link
Member

drujensen commented May 13, 2018

@faustinoaq I think we need to discuss our goals with Amber and I don't see hosting and maintaining recipes as one of them.

IMO the Amber team does not have the band-width or time to maintain recipes for possibly hundreds of different flavors of front-end and back-end combinations.

If we do decide to support the ability to mod amber, this should be an exercise outside of the core project. This is why I think shards is a better approach. Your example project amber-saludo is the right way to do this IMO.

I understand the issue with bootstrapping a new project using shards and here is my proposed solution.

I think we should be able to start a project using crystal init app blog and then add the amber shard to it. Then running a bin/amber init command will allow us to bootstrap a project and convert it to an amber project.

With this in mind, I think any modifications to the templates (recipes) could simply be adding a shard to the project before running the amber init command.

There are many advantages to this approach:

  1. We no longer maintain multiple flavors of amber recipes. We can remove the -m and possibly -t flags and keep the code base at a maintainable level.
  2. Anyone can create a new flavor of amber template by forking our base recipe repo and modifying it.
  3. We remove hosting recipes in the cloud and zip files as a dependency.
  4. We remove maintaining different flavors of templates and duplicate code.
  5. We can possibly remove maintaining amber as a standalone application from brew and apt.
  • AND
  1. You can add or modify only the generators you are interested in. You don't have to copy the whole recipe.

I would like to make sure we stay focused on the goals we set for Amber and I don't see maintaining multiple flavors of different recipes as one of those goals.

@damianham
Copy link
Contributor

@drujensen I agree that recipes are beyond the original goals of Amber and maintaining many blessed recipes could become a time consuming task. However, the fact that we curate blessed recipes also gives us the ability to ensure they stay up to date and perform well with no nasty surprises. I based the idea and implementation a little bit on Yo http://yeoman.io/ but made it a bit easier to create an app from a recipe by naming the recipe and downloading on the fly rather than the Yo method of installing a generator as an npm module. Once you have an amber binary on the path creating a new app from a recipe is very simple and does not involve pre-installing generators. You seem to be in favour of the Yo method of installing the generator before use.

There is one way in which we could have the simplicity of the recipe name as an arg to the new app generator while removing blessed curated recipes from the core project and that is to have a shard which converts the recipe name to a URL to the recipe zip file which could be anywhere on the web. #764 adds support for a recipe URL but as a user of the recipe feature I would rather type

amber new myapp -r basic/granite

than

amber new myapp -r https://raw.githubusercontent.com/amberframework/recipes/master/dist/basic/granite
or
amber new myapp -r https://raw.githubusercontent.com/damianham/recipes/master/dist/basic/granite

hence from my point of view, curated recipes, either in their current form or via a shard that converts the name to a URL, are a much easier option for end users.

@faustinoaq
Copy link
Contributor

faustinoaq commented May 13, 2018

I think we need to discuss our goals with Amber and I don't see hosting and maintaining recipes as one of them.

@drujensen Yeah, no problem, Indeed, I suggested some time ago we should use crystal init app instead of amber cli but I remember @elorest or @eliasjpr said a dedicated amber cli was a better idea. I think that's because our custom project structure and some other stuff, so is easier for us to use amber new and not crystal init.

IMO the Amber team does not have the band-width or time to maintain recipes for possibly hundreds of different flavors of front-end and back-end combinations.

I think this is not a problem Recipes can be maintained by community, also out blessed recipes are not hundred, just a few.

If we do decide to support the ability to mod amber, this should be an exercise outside of the core project. This is why I think shards is a better approach. Your example project amber-saludo is the right way to do this IMO.

Recipes doesn't modify amber, recipes are just pre-made amber projects with custom stuff. I think you can already use shards if you want to, but as I said before we would need to have at least a shard.yml file with "amber-plugin" dependency and we would need to compile again and again to use every new "plugin". Ex. amber-saludo. Also you will end with a bunch of binaries on bin directory (not very friendly)

I understand the issue with bootstrapping a new project using shards and here is my proposed solution....

@drujensen Yeah, I like your proposed solution (as I said before, I already proposed that some time ago) ,and I think you are a it confused 😅 We already concluded that amber cli on brew/aur/other was required, and using crystal init was a bit harder to use because amber project structure.

...I would like to make sure we stay focused on the goals we set for Amber and I don't see maintaining multiple flavors of different recipes as one of those goals.

Recipes is not against Amber goals. This is just a nice alternative to manage custom generators and cleanup a lot of templates we already have on amber itself.

Recipes are nice because:

  • Are automatically tested and deployed (Please see Add automatic deploy recipes#14 )
  • Are cached
  • Are very useful to support basic recipes and popular recipes to speed up amber development.

Is way easier to use react recipe, than create an amber project from scratch and figure out how to configure all front-end stuff.

This will allow us to cleanup a lot of default front-end files in current amber (a lot of web pack files and more) And now we would be able to use custom configuration, not only npm/webpack/boostrap but yarn, rollup, foundation, and other popular stuff.

I agree that recipes are beyond the original goals of Amber and maintaining many blessed recipes could become a time consuming task. However, ....

@damianham I think blessed recipes is not that hard to maintain. IMO is harder to maintain multiples repositores "shards", each one with multiples issues and PRs to track and review.

@damianham I agree, recipes are easy and useful, and with amberframework/recipes#14 are even easier to maintain and deploy

So, @drujensen WDYT? Can you give us access to Github Token to start using recipes/releases ? 😅

@imdrasil
Copy link
Author

@faustinoaq agree with a lot of mentioned stuff, but:

  • AFAIK I can't use both react recipe and crecto (e.g.) or any other recipe
  • a lot of binaries in bin folder IMO may be friendly enough if their launch is automated

@robacarp
Copy link
Member

@faustinoaq @drujensen I agree with targeting recipes with shards, and especially with the overlay idea that @drujensen has mentioned a few times. I see that as the "eventual destination" of this feature. Recipes is a big task that needs a lot of iteration before it's going to be perfect.

@imdrasil imdrasil force-pushed the add_jennifer_support branch 7 times, most recently from b938880 to c4d81b8 Compare May 20, 2018 21:35
@faustinoaq
Copy link
Contributor

@imdrasil Is this still work in progress?

@robacarp Yeah, I agree recipes can be shards, although, we need to test current implementation I'm already doing a lot of work on recipes to remove code duplication and add more tests about code formatting, ameba and crystal spec foreach recipe ✨

The final goal would be recipes as shards, but we still need some features like shards install drujensen/granite-recipe and so on, see: crystal-lang/shards#144. Currently recipes are being deployed and tested automatically, so I think the current recipes implementation by @damianham would be enough.

@imdrasil
Copy link
Author

@faustinoaq yeah and going to finish refactoring till tomorrow evening (my time). Sorry for such a long running PR - I got a lot of work to do on my work.

@faustinoaq
Copy link
Contributor

faustinoaq commented May 28, 2018

Hi @imdrasil I just created, amberframework/recipes#20, please take a look! 😉

BTW, I like some nice cleanup you did here, I think we can merge some code here (no Jennifer specific) 👍

@faustinoaq
Copy link
Contributor

Moved to amberframework/recipes#20

@imdrasil Thanks you!

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

Successfully merging this pull request may close these issues.

6 participants