-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
refactor(core): Clean up event relays (no-changelog) #10284
Conversation
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.
Nice work 👏 Just couple suggestions
|
||
@Service() | ||
export class AuditEventRelay { | ||
export class LogEventRelay extends EventRelay { |
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.
Could we name this LogStreamingEventRelay
to make it match the name of the feature it's related to (Log streaming)? LogEventRelay
might make me think it's about logging to console log
/** | ||
* Workflow | ||
*/ |
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.
Should we use VSCode foldable regions for these? E.g.
//
// #region Workflow
//
...
// #endregion
or
// #region Workflow
...
// #endregion
That way they can be folded. This is of course VSCode only feature, but personally I like it as a way to organize regions of code in files.
for (const [eventName, handler] of Object.entries(map) as Array< | ||
[EventNames, ((event: RelayEventMap[EventNames]) => void | Promise<void>) | undefined] | ||
>) { | ||
if (!handler) continue; |
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.
Is there a use case for leaving the handler
as undefined? I'd imagine it's a bug, so maybe it's better to explode here, e.g. by using an assert
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.
So I added undefined
in case someone forgets to specify a handler, but on second thought the type system will catch this oversight beforehand at call site, so we can simplify the type and remove the check.
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.
LGTM 🚀 Thank you for addressing the feedback 💟
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
✅ All Cypress E2E specs passed |
* master: refactor(core): Clean up event relays (no-changelog) (#10284) fix(editor): Fix execution retry button (#10275) feat(core): Show sub-node error on the logs pane. Open logs pane on sub-node error (#10248) refactor(core): Move instanceRole to InstanceSettings (no-changelog) (#10242) feat(core): Allow filtering executions and users by project in Public API (#10250) fix(core): Make execution and its data creation atomic (#10276) refactor(core): Mark schema env vars used by cloud hooks (no-changelog) (#10283) ci: Fix DB tests (no-changelog) (#10282) feat(core): Support create, delete, edit role for users in Public API (#10279) refactor(core): Decouple post workflow execute event from internal hooks (no-changelog) (#10280) feat(core): Allow transferring credentials in Public API (#10259) feat(core): Support create, read, update, delete projects in Public API (#10269) ci: Introduce lint rule `no-type-unsafe-event-emitter` (no-changelog) (#10254) fix(core): Surface enterprise trial error message (#10267) fix(editor): Enable moving resources only if team projects are available by the license (#10271) fix(core): Upgrade tournament to address some XSS vulnerabilities (#10277) # Conflicts: # packages/cli/src/Server.ts
Got released with |
Cleanup for event relays:
@/events
init
on both relaysPlease note this PR looks big but the majority is renamings - the only real changes are in:
packages/cli/src/events/event-relay.ts
packages/cli/src/events/log-event-relay.ts
packages/cli/src/events/telemetry-event-relay.ts
Follow-up to: #10280