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.

Sign up for GitHub

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

Jump to bottom

Remove BaseEventEmitter from components #4191

Merged
merged 9 commits into from
Jan 22, 2025
Merged

Remove BaseEventEmitter from components #4191

merged 9 commits into from
Jan 22, 2025

Conversation

ankur22
Copy link
Contributor

@ankur22 ankur22 commented Jan 17, 2025 •
edited
Loading

What?

Removing the BaseEventEmitter from Touchscreen, Page and Worker, as well as the events which are emitted.

Why?

While debugging another issue i found that these components had instances of their own event emitter, which were emitting events, but nothing was consuming them. It's confusing to see these events being emitted and can cause confusion while debugging issues with the event emitter. This is why they are being removed.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@ankur22 ankur22 requested a review from a team as a code owner January 17, 2025 10:47
@ankur22 ankur22 requested review from olegbespalov and joanlopez and removed request for a team January 17, 2025 10:47
@ankur22 ankur22 requested a review from inancgumus January 17, 2025 10:47
inancgumus
inancgumus previously approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

They are not needed 👍

joanlopez
joanlopez reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@joanlopez joanlopez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is that somehow "publicly" exposed? Like, can it be somehow used by k6 users?
Or those events are only being consumed internally by other components?

olegbespalov
olegbespalov reviewed Jan 20, 2025
View reviewed changes
js/modules/k6/browser/common/frame_session.go Outdated
@@ -661,7 +661,7 @@ func (fs *FrameSession) onConsoleAPICalled(event *cdpruntime.EventConsoleAPICall
}

func (fs *FrameSession) onExceptionThrown(event *cdpruntime.EventExceptionThrown) {
fs.page.emit(EventPageError, event.ExceptionDetails)
// TODO: Test and handle this
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if I'm getting this todo and also, does that make sense to have this empty body method at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, i think we need an issue for this as it may be something useful to build an API around. When the website throws an exception then the user might want to track this.

I don't want to delete it. Instead i might add some debug logging when this is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yeah logging that sounds reasonable, feel free to do this later, but it's worth maybe updating TODO within this PR to put more context for the future implementation 👍

js/modules/k6/browser/common/frame_manager.go Show resolved Hide resolved
js/modules/k6/browser/common/frame_manager.go Show resolved Hide resolved
js/modules/k6/browser/common/page.go Show resolved Hide resolved
@ankur22
Copy link
Contributor Author

ankur22 commented Jan 20, 2025

Is that somehow "publicly" exposed? Like, can it be somehow used by k6 users?
Or those events are only being consumed internally by other components?

@joanlopez These are all internal events, nothing exposed to the k6 user.

olegbespalov
olegbespalov previously approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

LGTM, like I said in-line it's worth providing more context for TODO, like Why? and optionally What?, but consider this non-blocking for merge

@ankur22
Remove BaseEventEmitter from worked
fb446ec 
The event that is emitted is never received anywhere else in the
codebase. Simplify the exec and code by removing the BaseEventEmitter.
@ankur22
Remove BaseEventEmitter from touchscreen
f8053c7 
The emitter is never used to emit anything. So remove it to simplify
the codebase.
@ankur22
Remove the event emit when session is closed
ce6e697 
Nothing is receiving the event when the session is closed.
@ankur22
Remove BaseEventEmitter from page
377d532 
The events which are emitted with the page's event emitter instance
weren't being received by anything else in the codebase.
@ankur22
Remove the events which aren't being used
7e2bdb2
@ankur22
Remove method that isn't needed
17e64b2
@ankur22
Remove unneeded logging
f95dfa0
@ankur22
Rename method to removeWorker
8a5aca7
@ankur22
Add debug logs in onExceptionThrown
638eb16
@ankur22 ankur22 merged commit a9854f1 into master Jan 22, 2025
23 of 29 checks passed
@ankur22 ankur22 deleted the remove/events branch January 22, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olegbespalov olegbespalov olegbespalov approved these changes

@inancgumus inancgumus inancgumus approved these changes

@joanlopez joanlopez Awaiting requested review from joanlopez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ankur22 @inancgumus @olegbespalov @joanlopez