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

Refactor frame.click option parsing #1173

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Jan 19, 2024

What?

This refactors the option parsing out of the promise and into the mapping layer for frame.click.

Why?

This will help mitigate the risk of a panic occurring due to multiple goroutines accessing the goja runtime (which is not thread safe) concurrently.

More works needs to be done to totally remove the goja runtime usage from within the frame.click promise which can be tracked in #1170.

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: #1172

@ankur22 ankur22 changed the base branch from main to main-next January 19, 2024 17:25
@ankur22 ankur22 changed the base branch from main-next to fix/locator-click-goja January 19, 2024 17:25
@ankur22 ankur22 force-pushed the refactor/frame-click-parse branch 2 times, most recently from c8fe5e4 to 8b21fdf Compare January 19, 2024 17:52
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

Thanks you for working on this immediately 👏

browser/mapping.go Show resolved Hide resolved
common/page.go Show resolved Hide resolved
Base automatically changed from fix/locator-click-goja to main-next January 22, 2024 12:28
This timeout method is to be called from the mapping layer when parsing
options.
This helps avoid using the goja runtime when it comes to parsing the
options. The options parsing has been moved to the mapping layer and so
has been moved back on to the main goja thread, which will mitigate the
possibility of a panic which could occur when multiple goroutines try
to work with the goja runtime (which is not thread safe).

There is another issue to tackle more of the goja refactoring in
#1170.

The change also adds a temporary fix for page.click which will also be
refactored at a later stage.
@ankur22 ankur22 merged commit 8d594a6 into main-next Jan 23, 2024
13 checks passed
@ankur22 ankur22 deleted the refactor/frame-click-parse branch January 23, 2024 14:04
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.

3 participants