Move goja option parsing out of frame and page goto opts #1179
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
inancgumus
changed the title
inancgumus
force-pushed
the
fix/goja-frame-page-goto-opts
branch
from
f21de98
to
52c351a
Compare
inancgumus
commented
Jan 22, 2024
inancgumus
commented
Jan 22, 2024
Comment on lines
+89
to
+91
| opts := &common.FrameGotoOptions{ | ||
| Timeout: common.DefaultTimeout, | ||
| } |
There was a problem hiding this comment.
page.Goto could implicitly set a default timeout. But I think this makes it explicit and less error-prone. WDYT?
There was a problem hiding this comment.
Yep, I was wondering the same. I went with the option of being explicit for now, and ensuring it matched the current behaviour. I think in a future refactor we can make it implicit.
There was a problem hiding this comment.
I'm on the side of keeping it explicit this way. This also gives transparent direct control to tests. Previously, we had our fair share of bugs because of time management and timeouts. Let's see :)
inancgumus
commented
Jan 22, 2024
inancgumus
commented
Jan 22, 2024
inancgumus
force-pushed
the
fix/goja-frame-page-goto-opts
branch
from
f1b5104
to
e04f7be
Compare
ka3de
approved these changes
Jan 23, 2024
Why? - Train of calls make the code less readable and maintain. - This is a precursor for calling the methods from the mapping layer while parsing the options with defaults.
Areas to consider: - frame and page gotos go hand to hand. we had to refactor both. - explicitness tests must now set their timeout goto options explicitly this was implicit before, potentially dangerous and can easily lead to unexpected behavior.
- This prevents the train-of-calls issue. - It might make it easier to navigate within the code for new contributors.
inancgumus
force-pushed
the
fix/goja-frame-page-goto-opts
branch
from
e04f7be
to
c97d901
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?
frameandpagegototo mapping.ReferrerandNavigationTimeoutfor building options while mapping.Why?
See #1162 for the main reason. Also, the reason for doing both
frameandpagegotooptions in the same PR is that the code doesn't compile without the other. Or, it would require a lot of intermediary steps which would be confusing for the commit log.Checklist
Related PR(s)/Issue(s)
Updates: #1176