Skip to content

Conversation

swilgosz
Copy link

@swilgosz swilgosz commented Apr 5, 2022

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

  • Create a slice per BC.
  • Instead of using global configuration forcing to enable all BCs at once, create config within each BC.

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.

  1. I have found some active support dependencies issues I needed to tweak.
  2. For some reason, when I configured ROM adapter, I ended up with an Sequel Error in RES-ROM and opened a quick-fix PR for it Additional value mapping (Hanami 2 integration) rails_event_store#1297

Sebastian Wilgosz and others added 18 commits March 12, 2022 13:51
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"
Copy link
Member

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.

Comment on lines +15 to +16
flash[:notice] = "Customer was already registered"
render "new"

Choose a reason for hiding this comment

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

Suggested change
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" }

?

@katafrakt
Copy link

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?

@andrzejkrzywda
Copy link
Contributor

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.

@swilgosz
Copy link
Author

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?

@katafrakt
Copy link

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.

@swilgosz
Copy link
Author

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 🙏

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.

4 participants