Prepare Page for async migration part #2
#1345
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
inancgumus
added
refactor
mapping
inancgumus
force-pushed
the
async/page-prepare-part2
branch
from
163eec3
to
944e00b
Compare
inancgumus
force-pushed
the
async/page-prepare-part2
branch
from
944e00b
to
ab24cee
Compare
inancgumus
force-pushed
the
async/page-prepare-part2
branch
from
ab24cee
to
1181a75
Compare
ankur22
reviewed
May 28, 2024
| @@ -948,21 +948,19 @@ func (f *Frame) FrameElement() (*ElementHandle, error) { | |||
| } | |||
|
|
|||
| // GetAttribute of the first element found that matches the selector. | |||
| func (f *Frame) GetAttribute(selector, name string, opts goja.Value) any { | |||
| func (f *Frame) GetAttribute(selector, name string, opts goja.Value) (any, error) { | |||
There was a problem hiding this comment.
GetAttribute should only returns a string. Having taken a quick look at the call stack it looks like we're using element.getAttribute, maybe a string instead of any?
It should also map to nil if the string is empty... not sure what the best way forward for that would be since an attribute could just be empty :/
There was a problem hiding this comment.
How about returning a bool to detect that in the mapping layer?
func (f *Frame) GetAttribute(selector, name string, opts goja.Value) (string, bool, error)
Even so, this should better go to a new issue because it'd change the existing signatures. I've tried to keep the signatures intact other than adding errors.
ankur22
approved these changes
May 28, 2024
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?
Turns panics into errors only for
Page's first set of methods. The rest will be in another PR.Why?
To make the async migration more reliable.
Checklist
Related PR(s)/Issue(s)
#1307