Skip to content

Conversation

metalwarrior665
Copy link
Member

@metalwarrior665 metalwarrior665 commented Jun 26, 2025

When working on apify/apify-sdk-js#404, I realized all event data are any, this is an attempt to correct that. It is just a draft idea with many problems:

  1. Tightening of types is breaking, even though the underlying JS objects don't change. So not sure how this fit into how you follow semver.
  2. I'm not too happy with the implementation, it doesn't feel extensible. On the other hand, this part of code will probably rarely change and we could refactor it if needed.
  3. We would shortly need to follow up with the SDK update for the PlatformEventManager

So take this more as a PoC idea. The reason I got to this is that I wanted to handle Actor.on('persistState') but do extra step if there is isMigrating or isAborting so not a huge use-case to have the types correct.

@metalwarrior665 metalwarrior665 requested a review from B4nan June 26, 2025 07:15
@janbuchar
Copy link
Contributor

Hi @metalwarrior665, is this a draft because it's still a work in progress or are you already asking for feedback?

export abstract class EventManager {
protected events = new AsyncEventEmitter();
export abstract class EventManager<SystemInfo> {
protected events = new AsyncEventEmitter<EventTypeToArgs<SystemInfo>>();
Copy link
Member

@vladfrangu vladfrangu Jun 26, 2025

Choose a reason for hiding this comment

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

One of these days we ought to move to having the class as the EventEmitter, but not today :D

import { EventManager, EventType } from './event_manager';

export class LocalEventManager extends EventManager {
interface LocalSystemInfo {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interface LocalSystemInfo {
export interface LocalSystemInfo {

@metalwarrior665
Copy link
Member Author

@janbuchar I'm asking for feedback. I kept it as Draft mainly to make sure we resolve the breaking change part first. Can make it Ready for review.

@B4nan B4nan added this to the 4.0 milestone Sep 2, 2025
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