search

palkan / active_event_store

Rails Event Store in a more Rails way
MIT License
181 stars 13 forks source link

Update to work with RES 2.3 #6

Closed ryanwood closed 3 years ago

ryanwood commented 3 years ago

What is the purpose of this pull request?

To update the gem to work with the current version of Rails Event Store (v2.3). Fixes #3.

This is not complete but is still a work in progress. It needs some help to get over the finish line.

What changes did you make?

I tried to get all tests passing (still one failure on a custom matcher) and followed the recommendations in https://github.com/RailsEventStore/rails_event_store/blob/master/railseventstore.org/source/docs/v2/custom_components_2_0.html.md.

I am considering using RES in a project and very much prefer the interface this gem provides so I wanted to attempt to get it working. That said, though I have 16 years of Ruby and Rails experience, I haven't previously used RES or this gem.

Is there anything you'd like reviewers to focus on?

Although my changes have gotten tests to pass, I'm not sure if they are really the optimal solution. Some things I noticed:

Checklist

palkan commented 3 years ago

I had to pass in serializer: JSON multiple times. That should probably be dried up in configuration setting.

Agree. Let's add a serializer config option and use it (i.e., (serializer: Config.serializer)).

I'd like to see the ability to store event data and metadata in JSONB fields vs. text fields

Yeah, I was thinking about it when I started using RES. And it seems that it now supports it (see, for example, this issue: https://github.com/RailsEventStore/rails_event_store/issues/651).

There is still a broken test within the custom matchers.

No problem, I'll take a look.

So, let's add a serializer configuration option, so we can merge it.

ryanwood commented 3 years ago

I've added the serializer to the config. RES also has a pretty clear docs on using JSON/JSONB fields along with the caveats here: https://railseventstore.org/docs/v2/mapping_serialization/#configuring-a-different-mapper.

Let me know if there is anything else you need from me.

palkan commented 3 years ago

Thanks! Let me merge it and fix the matcher.