-
Notifications
You must be signed in to change notification settings - Fork 126
Browser: Add view for list of event types #1920
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?
Conversation
Allow applications to provide custom event types query implementations while maintaining sensible defaults via DefaultQuery. Follows the same configuration pattern as related_streams_query.
Adds new page to browse all event types with links to their streams. Includes API integration, routing, and proper state management following existing Elm patterns. Also fixes duplicate event types in DefaultQuery by adding uniqueness check.
- Changed shared examples to accept query instance instead of class - Added expectation for result.size > 1 in shared examples - Added test to verify event_store is passed correctly to query factory - Achieved 100% mutation coverage for GetEventTypes 🤖 Generated with [Claude Code](https://claude.com/claude-code)
- Added test for filtering out classes without names - Added test for filtering out non-Event classes - Added test for stream name using class name format - Added test for deduplication of classes with same name Improved mutation coverage from 77.92% to 88.31% 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ormance Replace ObjectSpace iteration with recursive Class#subclasses calls: - More performant in production environments - Cleaner and more idiomatic Ruby code - Updated tests to stub subclasses instead of ObjectSpace 🤖 Generated with [Claude Code](https://claude.com/claude-code)
- Add test for nested subclasses recursion - Add test for alphabetical sorting of event types - Remove unused @event_store instance variable from DefaultQuery - 154/156 mutations killed (2 remaining are semantic equivalents) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
| def initialize(event_store); end | ||
|
|
||
| def call | ||
| all_event_subclasses(RubyEventStore::Event) |
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.
Something I don't like here is that with this approach it's not possible to override the event_type (or algorithm to build it) without replacing this Query object. Even though we in the end rely on the #event_type called on the event's instance level, it would be good to have something available on the class level for event type discovery. For example in Rails, when I have Payment < ActiveRecord::Base, I can call Payment.table_name to get the actual, low-level table name being used (I think it's a solid metaphor BTW).
Current implementation is quite hard-coded btw:
def event_type
metadata[:event_type] || self.class.name
end
Maybe it should be more in a vein of:
def event_type
metadata[:event_type] || self.class.event_type
end
? Then it would allow to override event type on the class level, not instance one.
That metadata[:event_type] was quite surprising too BTW 😆 (This MR doesn't consider this option for now)
| .select { |klass| !klass.name.nil? } | ||
| .sort_by(&:name) | ||
| .uniq(&:name) | ||
| .map { |klass| EventType.new(event_type: klass.name, stream_name: "$by_type_#{klass.name}") } |
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.
Also this stream name, should not be hardcoded
The PR adds the view for the list of event types available in the instance.
Inspired by https://github.com/orgs/RailsEventStore/discussions/1139
Screenshots
Screenshot, but one can also just run browser's devserver and go to http://localhost:9393/types
When functionality not enabled:
Caveats / challenges
RubyEventStore::Event. This allows that in the future additional information could be shared on this view (schema, event version, encryption properties, ...), but is a "convention over configuration" style. We have never declared that inheriting from theRubyEventStore::Eventis mandatory, however I treat it as "if you do it, you get the most additional features in the ecosystem". Similarly, you can use DB connection throughActiveRecord::Baseand do INSERT/UPDATE/whatever without an actualActiveRecord::Baseclass, yet by doing that you lose some of the additional functionalities which rails brings to the table.Another approach would be to fetch only the ones from the database. However going this way, I feel that actual possibilities are severely limited, since the database does not hold that much interesting information as the event classes and event store instance. For example, extension of current view could be displaying the subscriptions of the instance, etc., relying purely on data from the database seems unnecessarily limiting providing what experience we could bring to the Browser. Also, we go here into the area of DB performance problems, whether the queries on the indexing fetching the event types are fast enough etc.
Another approach could be a hybrid one, fetching the data with both sources. Slightly more complex with more room for error. However it could nicely cover the cases where event versioning is used, or there are events which were in the codebase, but are not there anymore. It would be a shame to not display them in this case.
experimental_event_types_query, default implementation is provided, but with this keyword -- it needs to be enabled explicitly, something which would allow us to break compatibility of this new view in minor versions. Similarly as it was done when introducingStepByStepFormatterfor RSpec.The UI can get opinions, I have not thought thoroughily on this, but until there is a bit more data than just a link, it's hard to think about providing better UX. In other words, I'm definitely open for suggestions, but I'd leave it for later.
Access. Currently one has to know a link :D Short approach would be to provide a link in the footer for now if this experimental feature is enabled, but I'd rather tend to provide a horizontal navbar on the top, below the current navbar. WDYT?
(minor), currently the initialization looks like this:
Because (I think?) we cannot be certain whether RES instance is initialized during mounting the browser. So either we do this, or we pass the `` and take
event_storefrom the `event_store_locator`, but then lock the API of the constructor in that QueryObject... I'm not certain what's better, I prefer passing built instances.WDYT?