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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core application #975

Merged
merged 35 commits into from Jan 19, 2019

Conversation

@jodosha
Copy link
Member

jodosha commented Dec 11, 2018

Features

TL;DR This is a rewriting of the core of Hanami

  • New application
  • New configuration syntax with defaults
  • Internal container
  • Unified routes
  • Delegate code reloading to hanami-reloader

Application

DRY-up settings

The big change from Hanami 1 is the removal of all the applications (web, admin) as a way to micro-configure areas of your project.

In the past we had:

  • config/application.rb
  • apps/{web,admin,...}/application.rb

To configure where to load files from, which was the default response format for each single app, the sessions, cookies, base URL, just to name a few.

The concept is to unify all these settings at the level of config/environment.rb, so they are propagated to the apps. DRY at its finest.

No more applications to hold settings

In order to not overwhelm newcomers, apps/{web,admin,...}/application.rb are gone.

Single apps can override the inherited settings from the main app, by tweaking settings in new concrete superclasses like Web::Action (apps/web/action.rb).

# frozen_string_literal: true

require_relative "./actions/authentication"

module Web
  class Action < Hanami::Action
    include Actions::Authentication
  end
end

Example

To make easier the transition from Rails, application term now refers to the main project. Here's an example of config/environment.rb:

# frozen_string_literal: true

module Soundeck
  class Application < Hanami::Application
    config.cookies  = { max_age: 600 }
    config.sessions = :cookie, { secret: ENV["SOUNDECK_SESSIONS_SECRET"] }
    config.middleware.use Middleware::Elapsed, "1.0"

    config.environment(:production) do |c|
      c.base_url = ENV["SOUNDECK_BASE_URL"]
      c.logger = { level: :info, formatter: :json }
    end
  end
end

馃攳 I'm considering to rename the file from config/environment.rb to config/application.rb. Please let me know what you think.

Configuration

I introduced a new syntax for the application configuration based on getters and setters, instead of the DSL we had in Hanami 1.

Before

cookies max_age: 600

After

config.cookies = { max_age: 600 }

In some cases like this it's a little uglier (IMO), but it helps to simplify the internals, by explicitly separating the intent of reading a value from the intent of setting it.

Hanami::Configuration now performs atomic/thread-safe operations both at the read and write time.

Defaults

Another difference from the past is that configuration now ships with defaults, instead of making everything explicit in the generated app.

Let me give you an example:

Before

This was an explicit setting of each application configuration.

# apps/web/application.rb
security.x_frame_options = "DENY"

After

Now the configuration has that value as a default, so the generated code is empty:

In case a developer needs to change the default, they will do it for the whole app:

# config/environment.rb
config.security.x_frame_options = "sameorigin"

Again, the goal is to simplify and not overwhelm newcomers.

Implemented settings:

  • environment: to yield env based settings ( config.environment(:production) { ... })
  • base_url: to set the base URL of the app (config.base_url = "https://example.com")
  • logger: to set the logger options (config.logger = { level: :info, format: :json })
  • routes: to set the path to routes file (config.routes = "path/to/routes")
  • cookies: to set cookies options (config.cookies = { max_age: 300 })
  • sessions: to set session options (config.sessions = :cookie, { secret: "abc" })
  • default_request_format: to set the fallback for request format (aka MIME Type) (config.default_request_format = :json)
  • default_response_format: to set the default response format (aka MIME Type) (config.default_response_format = :json)
  • middleware to mount Rack middleware
  • security to set security settings (see below)
  • inflections to configure inflections (see below)

Security settings:

  • x_frame_options: defaults to "deny" (config.security.x_frame_options = "sameorigin")
  • x_content_type_options: defaults to "nosniff" (config.security.x_content_type_options = nil)
  • x_xss_protection: defaults to "1; mode=block" (config.security.x_xss_protection = "1")
  • content_security_policy: defaults to "form-action 'self'; frame-ancestors 'self'; base-uri 'self'; default-src 'none'; script-src 'self'; connect-src 'self'; img-src 'self' https: data:; style-src 'self' 'unsafe-inline' https:; font-src 'self'; object-src 'none'; plugin-types application/pdf; child-src 'self'; frame-src 'self'; media-src 'self'" (config.security.content_security_policy[:style_src] += " https://my.cdn.example" for adding another source) (config.security.content_security_policy[:plugin_types] = nil to override the settings)

Please note that nil values will have the effect of NOT sending the corresponding HTTP response header. From the examples above X-Content-Type-Options won't be sent.

Setting a nil value to config.security.content_security_policy element, it will remove it from the header. From the example above plugin-types WON'T be part of Content-Security-Policy value.

Internal Container

I built a new internal container based on dry-system.

The potential of dry-system in Hanami context is to make possible for third-party plugins to register themselves and to configure the settings of an application.

It's of course thread-safe, for each component we can define dependencies. It's lazy: we can boot only a portion of the needed components, or we can tell it to boot up everything, which is useful when the app starts with the hanami server command.

It has for now the following components:

  • root: holds the absolute path to the root of the app
  • env: loads ENV vars from a .env
  • lib: recursively requires all the files under lib/
  • environment: requires config/environment.rb
  • logger: setup logger, according to the user settings
  • routes: requires config/routes.rb (new addition, see below)
  • apps: register applications defined in config/routes.rb
  • actions: requires apps/{web,admin,...}/action.rb, register action settings for each app
  • code: requires all the code from apps

Removed shotgun

Removed shotgun as way to reload the code.

The hanami gem won't even know about code reloading anymore. hanami new will generate a Gemfile with hanami-reloader gem, which is able to handle code reloading from the outside.

This decision is made because of historical issues regarding shotgun:

  • It took years to find a fragile solution to make it to work in the Hanami context
  • It doesn't work with JRuby and webconsole gem
  • It was cluttering the internals of the framework

Removed .hanamirc

Another step towards the simplification of the settings is the removal of .hanamirc. We used it to hold the following settings:

  • project: the project name, which can now be inferred from the lib/ namespace
  • template: the template engine (e.g. erb), which we want delegate for the future to external gems
  • testing: the testing framework (e.g. rspec), same as template.

Routes

Simplification again. In the past we had several places to configure the whole routes of an app: apps/{web,admin,...}/config/routes.rb. Now these files are gone in favor of config/routes.rb at the root of the project.

Thanks to hanami-router 2, we can define all the routes for all the apps in a single shot:

# frozen_string_literal: true

Hanami.application.routes do
  mount :web, at: "/" do
    root to: "home#index"
  end

  mount :admin, at: "/admin" do
    root to: "home#index"
  end
end

Please note the differences from the past:

  • No more multiple files and multiple routers instances for multiple apps
  • Single router for the whole app 馃檶
  • Mounting apps is no longer delegated to the gone Hanami.configure, which was cluttering config/environment.rb
  • Apps as concrete classes are gone (e.g. Web::Application), in favor of symbols
  • The apps registered with mount are the ones that are activated in the internal container

Inflections

Inflections are configured at the app level and they are propagated to all the components (mainly router and model in the future). We'll use dry-inflector for the purpose.

Here's an example:

module Soundeck
  class Application < Hanami::Application
    config.inflections do |i|
      i.plural "virus", "viruses"
    end
  end
end

Available commands

Because I'm rebuilding the core from the scratch, for this PR, only the following commands are available:

  • hanami version
  • hanami server

How to test it

I created a testing app so you can see how Hanami 2 is taking shape: https://github.com/jodosha/soundeck

jodosha added some commits Nov 16, 2018

@jodosha jodosha added this to the v2.0.0 milestone Dec 11, 2018

@jodosha jodosha self-assigned this Dec 11, 2018

@jodosha jodosha requested review from kaikuchn , mereghost and timriley Dec 11, 2018

@solnic

This comment has been minimized.

Copy link
Member

solnic commented Dec 11, 2018

馃攳 I'm considering to rename the file from config/environment.rb to config/application.rb. Please let me know what you think.

+馃挴 environment is too ambiguous IMO, whereas application says what it is

@sandelius

This comment has been minimized.

Copy link
Contributor

sandelius commented Dec 31, 2018

Wow, this is a huge change. I kinda like the whole container architecture we have now tho. But I can see how that can be a bit overwhelming for newcomers.

@jdickey

This comment has been minimized.

Copy link

jdickey commented Jan 4, 2019

One of the things that first drew me to Hanami was its approach to "monolith first". Yes, start your app as a monolith, but have the framework make it trivial (in the 1.x versions) to extract logic first to a separate app under apps/whatever and then, if desired, to an entirely separate codebase/Gem that the "original" app used.

By simplifying things for the Rails refugees (and I understand them; I was one recently), how is that ability going to be affected? Of course it's not used heavily yet; Hanami is still a new framework seen as "risky" and "experimental" in a lot of Rails shops. But by encouraging that aspect of Clean Architecture, Hanami was making promises about a cleaner, more maintainable future once large apps started to switch over to it. Whither Clean Architecture now? Or am I fundamentally misunderstanding things again?

@cllns

This comment has been minimized.

Copy link
Member

cllns commented Jan 4, 2019

One of the things that first drew me to Hanami was its approach to "monolith first". Yes, start your app as a monolith, but have the framework make it trivial (in the 1.x versions) to extract logic first to a separate app under apps/whatever and then, if desired, to an entirely separate codebase/Gem that the "original" app used.

By simplifying things for the Rails refugees (and I understand them; I was one recently), how is that ability going to be affected? Of course it's not used heavily yet; Hanami is still a new framework seen as "risky" and "experimental" in a lot of Rails shops. But by encouraging that aspect of Clean Architecture, Hanami was making promises about a cleaner, more maintainable future once large apps started to switch over to it. Whither Clean Architecture now? Or am I fundamentally misunderstanding things again?

The multiple apps still exist, but the configuration for the apps is unified into a single-file. It streamlines things.

@jdickey

This comment has been minimized.

Copy link

jdickey commented Jan 4, 2019

@cllns So, say, extracting one app into a separate Gem (for reuse with other related apps) would be pretty much as with 1.3, then? The only thing touched in the pre-existing app would be the appropriate bits of config ripped out and copied over, and the Gem hooked in to routes and such? That's certainly still cleaner than it could have been, or that I misunderstood it as being. Thanks for the clarification. I'll have to watch Luca's video again.

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Jan 4, 2019

I want to ensure that "monolith first" will stay. There was probably a misunderstanding in my explanation.

DRY up the setting doesn't mean to sacrifice the separation of concerns in your project.
New apps will be less verbose in their settings.

In the past we had "everything in your face" approach. What you see is what the framework does. But that was redundant in most cases.
The new approach is: framework has defaults, you can override for the whole project, or for the single app.

This is less intimidating for newcomers: Ruby newbies or "Rails refugees".
I hope this clarifies the intent.

@timriley
Copy link
Member

timriley left a comment

This is a great start, @jodosha, nice work :)

I like the philosophy you're following around hiding defaults, and removing unnecessary code from the generated app. I also think the renaming to "application" and "slices" is brilliant. They're obvious and memorable names, and most importantly, obviously distinct from one another. (I've been using "umbrella" and "sub-app" terminology to describe similar things, and quite frankly, it's awkward and unnatural feeling).

Regarding specifics, I've left some thoughts/questions for you in the code.

And I do have a few broader questions, too:

Removed .hanamirc

I couldn't see where this was done? I was trying to find a place where you were inferring the application name via scanning lib/, but I couldn't find it? Did I miss it?

Security settings

In your youtube video, you said these could be overridden on the per-slice base Action classes. How would you see that actually working, in practice?

Show resolved Hide resolved lib/hanami/container.rb Outdated
Show resolved Hide resolved lib/hanami/container.rb
Show resolved Hide resolved lib/hanami/routes.rb
end
end

def self.app

This comment has been minimized.

@timriley

timriley Jan 7, 2019

Member

I wonder if we want a different name for this, since the pairing of application/app may be a little confusing?

This comment has been minimized.

@jodosha

jodosha Jan 7, 2019

Member

@timriley This is the only instance of Hanami::Application subclass in the process. So it's the app.

Hanami.app 
# => #<Soundeck::Application:0x00007ffee940eee0 ...>

This comment has been minimized.

@timriley

timriley Jan 7, 2019

Member

Ah, so for a booted application, Hanami.application is the application's class (via the Hanami::Application.inherited hook) and Hanami.app is the one instance of that application class? Is that right? i.e.

Hanami.application
# => Soundeck::Application

Hanami.app
# => #<Soundeck::Application:0x00007ffee940eee0 ...>

I still feel like that's a pretty confusing pairing of names. "app" is just shorthand for "application", they're synonyms. It feels a little odd that we use them both here, holding 2 different (though obviously related) things.

This comment has been minimized.

@jodosha

jodosha Jan 8, 2019

Member

@timriley Which name do you suggest? 馃槂 Please remember that Hanami.app is short and memorable, and it's used by public facing integration specs.

Eg.

require "features_helper"

RSpec.feature "Signup" do
  let(:app) { Hanami.app } # for capybara and rack-test
end

This comment has been minimized.

@cllns

cllns Jan 8, 2019

Member

@jodosha What about project for the container application? You used "project" four times in the main message to refer to this :) It's not quite as memorible or short as app, but it's not hard to remember either. I agree with @timriley that using app and application as different things would be confusing.

This comment has been minimized.

@jodosha

jodosha Jan 9, 2019

Member

I'm not sure to if keeping project is a good idea. The intention is to remove the word "project" from the terminology. See my YT video.

This comment has been minimized.

@timriley

timriley Jan 9, 2019

Member

Maybe we could stick with app in both methods but disambiguate between the two with a suffix, e.g. #application and #application_class? (maybe paired with shorter #app aliases too)

This comment has been minimized.

@solnic

solnic Jan 10, 2019

Member

Given that app returns an instance of the application, I would say a natural name for the other method should be app_class. I would also say this should be a private API anyway.

This comment has been minimized.

@cllns

cllns Jan 10, 2019

Member

I can understand some of the confusion between "Hanami project" referring to this whole effort, but I still think "project" is a natural and obvious way to talk about what the "umbrella" app is.

Will apps/ be renamed to slices/? They're still Rack-compatible apps. While I like the naming of "slices" and it seems appropriate, I feel like this is re-inventing the wheel and causes more confusion than it's worth. Hanami.app (or Hanami.application) being a different thing from Hanami::App is confusing. If we want to use "slice", we should go all the way and rename Hanami::App to Hanami::Slice. I don't think we should do that, but we should be consistent if we're changing the terminology.

Django uses the project/ application distinction in the same way we have been: https://medium.com/@timmykko/a-quick-glance-of-django-for-beginners-688bc6630fab

Any Django project will have at least one Django app. Yes, I know, it鈥檚 a little confusing. A Django project encompasses the application and all its components, while a Django app is a sub-container of the application with its own functionality that, in theory, may be reusable in another application without much modification.

(Also, for others, here's the YouTube video, since it hasn't been linked in this PR yet)

This comment has been minimized.

@cllns

cllns Jan 14, 2019

Member

@jodosha Thoughts on the above? 鈽濓笍 (Don't mean to bother you, I just want to make sure you saw it since I didn't mention you in the comment.) 馃尭

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Jan 7, 2019

@timriley Regarding .hanamirc, won't be implemented anymore. It isn't part of this PR, but the information can be inferred like this:

  1. Project/App name: from the Hanami::Application subclass. Eg. Soundeck::Application, per convention, "soundeck" will be inferred from there.
  2. Template framework: from a setting that can be either in the view framework or in the project configuration
  3. Testing framework: I want to extract hanami-rspec and hanami-minitest, which will register themselves as testing framework and will be able to generate the proper testing files.
Show resolved Hide resolved lib/hanami/cli/commands.rb Outdated
Update lib/hanami/cli/commands.rb
Co-Authored-By: jodosha <me@lucaguidi.com>
@kaikuchn
Copy link
Member

kaikuchn left a comment

Whew, finally finished reviewing this! 馃槍
You did a great job with this. I especially love the unified routes!

Show resolved Hide resolved lib/hanami/cli/commands/server.rb Outdated
Show resolved Hide resolved lib/hanami/configuration.rb Outdated
Show resolved Hide resolved lib/hanami/configuration.rb
Show resolved Hide resolved lib/hanami/configuration.rb Outdated
Show resolved Hide resolved lib/hanami/configuration/security.rb
Show resolved Hide resolved lib/hanami/configuration/security.rb
Show resolved Hide resolved lib/hanami/configuration/sessions.rb
Show resolved Hide resolved lib/hanami/container.rb Outdated
Show resolved Hide resolved lib/hanami/routes.rb

jodosha added some commits Jan 9, 2019

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Jan 9, 2019

@kaikuchn Thanks for your feedback. I addressed all the feedback points. 馃憤

@davydovanton
Copy link
Member

davydovanton left a comment

awesome work 馃憦

Show resolved Hide resolved lib/hanami/container.rb Outdated
Show resolved Hide resolved lib/hanami/container.rb Outdated
@solnic
Copy link
Member

solnic left a comment

I left some comments about things that stuck out to me. Overall it's hard for me to truly review this diff because of its size and scope. I think a more reasonable approach (at least for me) would be to get this merged and play with it to see how things work.

There are a couple of things that I deeply care about and this will be my focus when I'm testing this out:

  • benefit from dry-system's booting mechanism, ie is it easy to write a test that boots only part of the app?
  • fast booting (either parts of the app or the whole app)
  • easy to leverage auto-injection with my own classes

As far as more hanami-specific stuff goes, I need to make myself more familiar with router/actions first in order to test it out and provide feedback.

Overall I'm extremely happy to see this. I will be repeating this over and over again - the approach that dry-system provides has been one of the major improvements I've experienced when building apps in Ruby and I'm 10000000% sure people will dig it eventually. It's a matter of making it easy to get started and I believe this is exactly what Hanami can provide.

Thanks Luca for your hard work! <3

Show resolved Hide resolved lib/hanami/container.rb
$LOAD_PATH.unshift root.join("lib")
Hanami::Utils.require!(c[:root].join("lib", "**", "*.rb"))
end
end

This comment has been minimized.

@solnic

solnic Jan 10, 2019

Member

This shouldn't be needed. Why blindly requiring all the lib files?

This comment has been minimized.

@jodosha

jodosha Jan 14, 2019

Member

This is for convenience sake. Given under lib/ there are repositories and entities, that are necessary for the app.

Do you think Auto-Registration should be used here instead? Or do you suggest to let devs to manually require code?

This comment has been minimized.

@solnic

solnic Jan 20, 2019

Member

@jodosha sorry for replying late - my preference is to "require what you require", with no exceptions. Probably a good solution in Hanami would be to offer "load everything" as an option that users can turn on/off. In such case folks coming from Rails would feel more comfortable when it's turned on.

Do you think Auto-Registration should be used here instead?

Partly yes. Auto-registration works great for all objects that are used as injectible dependencies. There are exceptions though - ie entities are not used in DI, so they shouldn't be auto-registered.

Show resolved Hide resolved lib/hanami/container.rb
Show resolved Hide resolved lib/hanami/container.rb Outdated

jodosha added some commits Jan 14, 2019

@jodosha jodosha merged commit ca2cc1e into unstable Jan 19, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jodosha jodosha deleted the feature/application branch Jan 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment