Moved Element options and Frame options (partially) to mapping layer #5329
Conversation
anupriyakkumari
requested review from
ankur22 and
codebien
and removed request for
a team
|
|
anupriyakkumari
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
anupriyakkumari
temporarily deployed
to
azure-trusted-signing
GitHub Actions
Inactive
anupriyakkumari
force-pushed
the
move-ElementHandleBaseOptions-mapping-layer
branch
from
52bf2b8 to
df02a1d
Compare
anupriyakkumari
force-pushed
the
move-ElementHandleBaseOptions-mapping-layer
branch
3 times, most recently
from
6679d1d to
16cab73
Compare
There was a problem hiding this comment.
Thanks for the changes 🙇 It's currently difficult to review this PR, and we might miss something without knowing it. Please split the current large commit into separate commits for each option (as long as they don't depend on each other). We should also resolve conflicts and make sure tests pass.
| } | ||
|
|
||
| // parseElementHandleBaseOptions parses ElementHandleBaseOptions from opts | ||
| //nolint:unparam // keeping error for consistency with other parse functions |
There was a problem hiding this comment.
Please remove error returns from all functions that don't return errors, as we don't need them now. We can add errors later if needed. We don't need to be consistent here.
| optionModifiers = "modifiers" | ||
| ) | ||
|
|
||
| const noWaitAfterOption = "noWaitAfter" |
There was a problem hiding this comment.
Please move this into the same const block and prefix it with option:
| optionModifiers = "modifiers" | |
| ) | |
| const noWaitAfterOption = "noWaitAfter" | |
| optionModifiers = "modifiers" | |
| optionNoWaitAfter = "noWaitAfter" | |
| ) |
| var imageFormatToID = map[string]common.ImageFormat{ //nolint:gochecknoglobals | ||
| "jpeg": common.ImageFormatJPEG, | ||
| "png": common.ImageFormatPNG, | ||
| } |
There was a problem hiding this comment.
Please remove nolint and move this to parseElementHandleScreenshotOptions, as it's only used there.
| "selectText": func(opts sobek.Value) (*sobek.Promise, error) { | ||
| popts := common.NewElementHandleBaseOptions(eh.DefaultTimeout()) | ||
| if err := popts.Parse(vu.Context(), opts); err != nil { | ||
| popts, err := parseElementHandleBaseOptions(vu.Context(), opts) |
There was a problem hiding this comment.
This should take a timeout as a parameter, as before. Please apply this logic to other parse functions as well.
We don't want to change any existing behavior.
| parsed := &common.ElementHandleBaseOptions{ | ||
| Force: false, | ||
| NoWaitAfter: false, | ||
| Timeout: 0, |
There was a problem hiding this comment.
We should get Timeout from an input. Applies to other functions as well.
|
@anupriyakkumari We'll release k6 1.4.0 in two weeks. Please continue working on this PR if you want it merged into k6 1.4.0. Otherwise, we'll have to postpone merging your PR until k6 1.5.0. I might work on some of the parts of #5305 even if you can't continue working on it. |
Hi @inancgumus. Sorry for the delay, I am caught up in some work till 30th but I will trying my best to at least finish work on this PR. |
3 participants
What?
Worked on #5305
This PR partially moves Sobek option parsing to the mapping layer, removing parsing logic from the business logic layers.
Specifically, this PR includes moving the following option parsers:
ElementHandle{...}OptionsFrameoptions dependent on element optionsThis has resulted in changes across multiple mapping layers in the browser package.
Why?
Related
This change is part of the ongoing effort outlined in the epic issue:
Move Sobek entirely out of the business logic #4219
Checklist
make check) and all pass.