Refactor elementHandle.click parse #1171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankur22
force-pushed
the
refactor/elementHandle-click-parse
branch
from
3228f1d to
e1e13bf
Compare
ankur22
force-pushed
the
refactor/elementHandle-click-parse
branch
2 times, most recently
from
44eeb8d to
44440af
Compare
ka3de
approved these changes
Jan 22, 2024
| "click": func(opts goja.Value) (*goja.Promise, error) { | ||
| ctx := vu.Context() | ||
|
|
||
| popts := common.NewElementHandleClickOptions(eh.Timeout()) |
There was a problem hiding this comment.
nit: What does popts stand for? Should it be clickOpts?
There was a problem hiding this comment.
I think copts would be fine for "click options".
There was a problem hiding this comment.
popts -> parsed options. Aren't opts before being parsed also click options? I prefer popts.
There was a problem hiding this comment.
popts and copts are both fine to me.
inancgumus
approved these changes
Jan 22, 2024
This will be used in the mapping layer to retrieve the default timeout that needs to be used when parsing options.
Move the parsing of the options for elementHandle.click outside of the promise goroutine and back onto the main goja thread in the mapping layer. This will help mitigate the risk of a panic if more than one goroutine is accessing the goja runtime off the main goja thread. This doesn't solve the problem completely though since this API calls to other areas of the codebase which does still interact with the goja runtime. See #1170 for further details.
ankur22
force-pushed
the
refactor/elementHandle-click-parse
branch
from
44440af to
f8f0f35
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Move the parse logic for
elementHandle.clickinto the mapping layer to reduce the call to the goja runtime by one less call.Why?
This will mitigate the risk of a panic occurring due to using the goja runtime in a new goroutine off the main goja thread. This doesn't solve the complete problem and it still relie on using the goja runtime, but that issue can be tracked separately (#1170).
Checklist
Related PR(s)/Issue(s)
Updates: #1169