-
Notifications
You must be signed in to change notification settings - Fork 77
Hanami integration #110
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
base: master
Are you sure you want to change the base?
Hanami integration #110
Conversation
In Hanami 2.0 it is very natural to create slices for different bounded contexts. We can have also bigger slices, for UI that aggregates multiple contexts together.
activerecord-import is not used anywhere, but it kind of forces rails dependency rails_event_store is not used anywhere, everywhere just ruby_event_store is used.
The whole ROM configuration is already done in hanami, so I just copied the file
In some environments (i.e Hanami app), gems are not loaded by default
For some reason configuring repositories in Hanami app, I get hash data and metadata in the payload within the Event Relation. Fork contains a quick fix to allow harmless integration
General ecommerce configuration does not need to be used directly. It enables all contexts at once, but from Hanami perspective, it is better to enable each context within proper slice.
@@ -1,3 +1,4 @@ | |||
require "active_support/isolated_execution_state" | |||
require "active_support/core_ext/hash" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to include this? It has been already reverted once so I'd like to see the backtrace why it reappears.
flash[:notice] = "Customer was already registered" | ||
render "new" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flash[:notice] = "Customer was already registered" | |
render "new" | |
res.flash[:notice] = "Customer was already registered" | |
render "new" |
But does it make sense to set flash
when it's never read?
Also, render "new"
does not work, but since this is API-only for now, shouldn't it be something like:
res.body = { error: "Customer was already registered" }
?
RailsEventStore/rails_event_store#1297 (comment) Co-authored-by: Paweł Świątkowski <katafrakt@users.noreply.github.com>
I have been recently wondering what is the status of this initiative. Almost 3 years later Hanami is much more complete framework, includes persistence etc. Most of code specific to RES/ecommerce will likely still work without changes, but the base Hanami app would look differently (for example, using eslint instead of webpack). Would it make sense to create a new PR with empty Hanami 2.2 app and apply the commits from this branch on top? |
It would be nice to see it working with the new Hanami. I dont know enough about its internals to suggest which way to go. My gut feeling is to start from scratch. |
In that period I also gained much more expertise on Hanami, I will re-add this to my schedule for next month, would that work for you? |
Hey @swilgosz, sorry if it feels pushy. I just wanted to check if this is still something on your roadmap. Asking because I might have some extra time in the near future and Hanami integration could be one of the things to spend it on. But I definitely don't want to duplicate efforts or dismiss your work, of you still have your eyes on this. |
Hey, thanks for mentioning this. Unfortunately, this completely slipped-off of my mind. I will appreciate if you can just check it off, but let me know if you can't. I hate leaving things unfinished, so may take care of it just to stay sane. Let's stay in contact 🙏 |
Overview
Integration of Ecommerce with Hanami 2 application.
The code includes the hanami 2 initial app files, so it can be a bit hard to review all. This is why for now I only integrated CRM BC as it seemed to be the smallest one.
Design
Dev notes
To review PR without initial Hanami 2 app files, here is the comparison skipping initial commits.: swilgosz/ecommerce@9afec8f...swilgosz:hanami-integration
Caevats.