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

Apps in gems #1007

Merged
merged 18 commits into from Jan 29, 2013
Merged

Apps in gems #1007

merged 18 commits into from Jan 29, 2013

Conversation

skade
Copy link
Contributor

@skade skade commented Jan 12, 2013

Hi everybody,

I took the liberty of finally coming up with a system to wrap apps in gems. Its an often requested feature.

The basic version turned out easier than expected - its basically just a spare Padrino app that can be run both from the gems repository as well as loaded from other Padrino locations. It only contains backwards-compatible changes to the mounter.

It comes with a generator for quickly generating valid Padrino app gems - just run padrino-gen gem my_gem_name with all the known options and be done.

Some limitations apply, which can or cannot be fixed by the framework:

  • config/ in gems obviously only makes sense for testing the thing or running it standalone.
  • public_path is currently the mouting apps public path.
  • bringing assets from the mounted app to the main app is generally an unsolved problem. This sounds like a job for sprockets or similar.
  • Tests for the generator are missing (the mounter is tested)
  • Obviously, care has to be taken with ORM dependencies if you want to build a generally usable App.
  • The README file for the generated Gem is currently empty, I'd like to fill it with some hints and pointers.

For API use, for example, this implementation is fully usable.

What do you think? @achiu, @nesquena , @dariocravero ?

Regards,
Florian

If Padrino is loaded using rubygems, this patch enables Padrino::Mounter to load apps from gems. To
allow the reloader what it does, this only searches for the app file, but does not load the
application. Applications in gems are expected to be found in `gem-folder/app/app.rb`. Gems named
after `ApplicationConstant.underscore` will be found. Optionally, they can be prefixed with
`padrino-`.
This allows application objects nested one level into a Module to be autodetected. If a module
contain more than one app, the behaviour is undefined.
This generator generates a spare Padrino application that works both as a standalone application and
as a mountable Gem in other probjects.
@dariocravero
Copy link

I think this is just brilliant @skade!!!! Thanks sooo much!! 👏

I'm definitely seen myself reusing APIs!! Imagine the power this brings!.. We can come up with a set of common API gems and bundle them as their own gem-mountable-apps. Amazing!!

As for sprockets and the like, I'd be inclined to use Grunt for the job -yes, I know, it's not Ruby but it I've found it does the job way better and we can then take advantage of Bower and the like. The asset pipeline will kick ass then! This isn't strictly related to the problem you're describing but it's something I've applied in some projects and it just works very well! See a custom version of an asset serving thing here it doesn't make loads of sense without the Gruntfile though. I'll explain better soon.

@achiurizo
Copy link
Member

thanks @skade, looks like a great idea and a great start to it! I also noticed you did nested apps in modules. I feel like it would be better just to default it and have the apps generate it namespaced for you per #741 ? This saves us the trouble of having the mounter handle the discovery. I'll try to get that feature out ASAP finally so we have something more concrete to play with.

@dariocravero
Copy link

👍 on namespacing!

@skade
Copy link
Contributor Author

skade commented Jan 14, 2013

Actually, this patch only adds a special behaviour that allows Mounters to find one app in a given module.

The reason for the namespacing is actually bit non-obvious and not related to the wish to namespace apps in general. I used the gem template from bundler as a reference, which generates a module named like the gem and puts an additional VERSION constant in it. The problem is that if the app name would match the gem, we would "half-load" the app constant when bundler loads the gem. This is a situation I want to avoid at all costs. Using Foo::App as a default constant, this is cleanly circumvented. If the module is mounted, the mounter can still derive the gem name from that name. Another option would be to recommend to mount "Foo::App" explicitely and use the module name to find the gem, which would remove the mounter magic. I think this is the better option - I'll implement it.

Interesting detail: the branch only fails on Travis because of timeout problems. https://travis-ci.org/padrino/padrino-framework/jobs/4108983

This change expects that all app names are given in full. No magic detection of apps in modules -
this has too many implicit assumptions, like there only being one app in a module.
Also, make all require_paths potential app locations. While this strictly includes 'lib', this means
that multiple apps can be packaged in a gem in a sane way.
@skade
Copy link
Contributor Author

skade commented Jan 14, 2013

I added a few changes that:

  • allow multiple apps to be loaded from a gem by listing them in the gemspec
  • removed the detect-app-in-module hackery
  • specify the gem name, should it differ

Given how full-circle this feature has come, generating a project as gemable by default and skipping on the whole gem generator thing is actually an option. Any opinions on this?

This allows the project generator to generate a gemspec fitting the project add some files to assist
in gem development.
This avoids accidentally finding the wrong app that comes earlier in the require-path.
@nesquena
Copy link
Member

Yeah nice I like the idea of just having this as an option flag to make it even easier to generate as a gem. Want to spend some time looking at this a little more and hopefully helping us to write some documentation for it as well closer to the release.

This adds a Padrino::Module module that orchestrates loading of gems.

Padrino::Module implements the most important methods to properly load code from a gem
(#dependency_paths and #root) and adds a method `gem!` to flag a module as being loaded from a gem.

It also modifies the generators so that now, everything works out of the box.
@skade
Copy link
Contributor Author

skade commented Jan 21, 2013

I made a few fancy additions. The gems main file now has to contain:

require 'padrino-core'

module MyGem # this can actually be anything
  extend Padrino::Module
  gem! 'my_gem' # this has to be the proper gem name and is actuall an alias for Padrino.gem('my_gem', MyGem)
end

After that, we have the following:

MyGem.root #=> "/path-to-rubygems/gems/my_gem-0.1.1/"; accepts args like Padrino.root
MyGem.dependency_paths #=> Like Padrino.dependency_paths, is automatically included in the latter now

This allows us to detect apps in gems without looking at all gems (inefficient and implicit) and gives us the gem name without black magic.

First of all, I found rails detection of engine gems always very strange. AFAIK, it analyses the call stack on load of the Engine class and tries to implicitely detect the name of the gem or similar magic. This approach gives the user direct control when the registration to the core system happens and makes it possible to implement Padrino::Modules without actually loading them from gems. Currently, the implementation is gem-bound, but this is just an internal detail.

Also, in contrast to previous versions, gems now have a sensible set of dependencies that mirrors the normal ones.

@skade
Copy link
Contributor Author

skade commented Jan 25, 2013

Okay, I am done with this feature in the current state, so this can be merged.

Some notes about the future:

  • The module system can be used to build upon to migrate current apps into modules instead of using the Padrino module directly. Its very lean. I'll create a ticket for that later on.
  • Gemifying all apps (including the main project) might be a thing. This would actually make a gem and an app no different at all. While the advantages may not be directly apparent, I actually had such a case a few weeks ago in a project, when an attempt was made to build a styleguide-app out of multiple present apps. I'll open a ticket for that and maybe build a demonstration... but only with...
  • We really need an asset pipeline of some sorts to allows gems to bring javascript, sprites and all.

Travis failed to check out the repos for the jruby test.

@achiurizo
Copy link
Member

@skade I think the gemified apps feature is great. However I'm still not keen on having the mounter do any of the work behind the scene to assist with the mounting. I do like however the ability to extract out the
application path from the gem via Padrino::Module. This allows me to mount things more explicitly, with multiple apps in gems as well:

Padrino.mount("MyGem::Project",  :app_file => MyGem.root('lib/app.rb')).to('/gallery')
Padrino.mount("MyGem::Project2", :app_file => MyGem.root('lib/project_2.rb')).to('/gallery2')

What do you think about that?

@skade
Copy link
Contributor Author

skade commented Jan 26, 2013

@achiu It depends on what you define as "behind your back". Nothing is mounted without you telling the mounter to do so - the only thing it currently sets up and additional root where it can find additional apps. As we expect those apps to be namespaced, name clashes are ruled out. IMHO, its not more or less behind my back then it currently is.

In the current form, the mounter will only find apps in gems if the gem has registered to do so (by calling gem!). By default, this happens when bundler loads the gem and can be deactivated by setting :require => false in the Gemfile.

Your code should work already.

I removed the code that auto-detects gems by name somewhere in the middle of the implementation.

@skade skade mentioned this pull request Jan 26, 2013
achiurizo added a commit that referenced this pull request Jan 29, 2013
@achiurizo achiurizo merged commit 5b9a6dd into padrino:master Jan 29, 2013
@skade skade mentioned this pull request Mar 18, 2013
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.

None yet

4 participants