Skip to content
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

feat: support nested event methods #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

andiwils
Copy link
Collaborator

@andiwils andiwils commented May 23, 2024

This PR adds an optional path parameter to the AddEvents utility, enabling the specification of nested event locations within objects. The addition of the getEvent closure function allows for retrieving events based on this optional path, and an error is thrown if the event is not found.

A successful example:

// Define a class with a nested event structure
class ExampleClass {
  nested = {
    onUpdate: new TypedEvent<() => void>()
  };
}

// Enhance the class with event handling capabilities specifying the path to the nested events
const EnhancedExampleClass = AddEvents<typeof ExampleClass, { onUpdate: TypedEvent<() => void> }>(ExampleClass, 'nested');

// Create an instance and attach an event listener
const exampleInstance = new EnhancedExampleClass();
exampleInstance.on('onUpdate', () => console.log('Event triggered'));

// Trigger the event
exampleInstance.nested.onUpdate.emit();

An error example:

// Attempt to create an enhanced class with an incorrect path
try {
  const InvalidExampleClass = AddEvents<typeof ExampleClass, { onUpdate: TypedEvent<() => void> }>(ExampleClass, 'incorrectPath');
  const exampleInstance = new InvalidExampleClass();
  exampleInstance.on('onUpdate', () => console.log('This will not run'));
} catch (error) {
  console.error(error);  // Outputs: Error: Event "incorrectPath.onUpdate" is not defined
}

@andiwils andiwils requested a review from bbaldino May 23, 2024 21:46
Copy link
Contributor

@bbaldino bbaldino left a comment

Choose a reason for hiding this comment

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

couple minor comments. can you add a section to the README about this as well?

Comment on lines 101 to 104
{
eventOne: TypedEvent<(value: number) => void>;
eventTwo: TypedEvent<(value: boolean) => void>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency, can you extract these out into an interface like the test above does? with them matching it makes it easier to see that what's different here is the path argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing. Done.

Comment on lines 73 to 74
// Even though we bypass type safety in the call (casting this as any), we've enforced it in the
// method signature above, so it's still safe.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks like it isn't really relevant here anymore...maybe better as a note on getEvent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Done.

@andiwils
Copy link
Collaborator Author

@bbaldino Thanks for the feedback. I fixed the tests to be more consistent, moved the stale comment into the comment getEvent block, and added a section in the readme.

@andiwils andiwils requested a review from bbaldino May 29, 2024 22:20
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.

2 participants