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

Fix goja escape from DispatchEvents #1193

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Fix goja escape from DispatchEvents #1193

merged 3 commits into from
Jan 26, 2024

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Jan 25, 2024 •
edited
Loading

What?

All DispatchEvent methods now:

  • Return errors.
  • Moves their option parsing to the mapping layer.
  • Prevent passing down Goja values.

Fixes tests:

  • TestLocator
  • TestBrowserOptionsSlowMo
  • The goja escape was related to DispatchEvents.

Why?

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #1182

@inancgumus inancgumus added bug Something isn't working tests refactor labels Jan 25, 2024
@inancgumus inancgumus self-assigned this Jan 25, 2024
@inancgumus inancgumus force-pushed the fix/goja-execctx-assert-test-locator branch from 5738545 to b0c7efc Compare January 25, 2024 11:21
@inancgumus inancgumus requested review from ankur22 and ka3de January 25, 2024 11:23
@inancgumus inancgumus mentioned this pull request Jan 25, 2024
Closed
@inancgumus inancgumus changed the title Refactor away goja from dispatch event Fix goja escape from DispatchEvents Jan 25, 2024
@inancgumus inancgumus force-pushed the fix/goja-execctx-assert-test-locator branch from b0c7efc to ddc9841 Compare January 25, 2024 11:25
Base automatically changed from fix/goja-execctx-assert to fix/goja-execctx-eval January 25, 2024 11:40
@inancgumus inancgumus mentioned this pull request Jan 25, 2024
Merged
3 tasks
@inancgumus inancgumus force-pushed the fix/goja-execctx-eval branch 2 times, most recently from b34470e to 3ed072b Compare January 25, 2024 11:57
@inancgumus inancgumus force-pushed the fix/goja-execctx-assert-test-locator branch 2 times, most recently from 5b01f1d to b5c4e98 Compare January 25, 2024 12:19
Base automatically changed from fix/goja-execctx-eval to main-next January 25, 2024 14:52
@inancgumus
Refactor away goja from dispatch event
06d14d1 
This is a necessary fix to remove passing Goja values to
ExecutionContext.
@inancgumus
Fix TestBrowserOptionsSlowMo dispatch event
0a34ed2
@inancgumus inancgumus force-pushed the fix/goja-execctx-assert-test-locator branch from b5c4e98 to 0a34ed2 Compare January 25, 2024 14:54
ankur22
ankur22 approved these changes Jan 25, 2024
View reviewed changes
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM.

I left a nit for you to consider, but not important.

tests/locator_test.go Outdated Show resolved Hide resolved
@inancgumus inancgumus merged commit 9759015 into main-next Jan 26, 2024
14 checks passed
@inancgumus inancgumus deleted the fix/goja-execctx-assert-test-locator branch January 26, 2024 06:37
@BrewTestBot BrewTestBot mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankur22 ankur22 ankur22 approved these changes

@ka3de ka3de Awaiting requested review from ka3de

Assignees

@inancgumus inancgumus

Labels
bug Something isn't working refactor tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@inancgumus @ankur22