Skip to content

Implement wait_for_selector - #236

Open
Mifrill wants to merge 13 commits into
mainfrom
issue_82/implement_wait_for_selector
Open

Implement wait_for_selector#236
Mifrill wants to merge 13 commits into
mainfrom
issue_82/implement_wait_for_selector

Conversation

@Mifrill

Copy link
Copy Markdown
Contributor

closes#82

@MifrillMifrill self-assigned this Jan 7, 2022
@Mifrill
Mifrillforce-pushed the issue_82/implement_wait_for_selector branch from 05e5d00 to bc13315CompareJanuary 7, 2022 00:45
@Mifrill
Mifrillforce-pushed the issue_82/implement_wait_for_selector branch from bc13315 to 83ca882CompareJanuary 7, 2022 00:51
@Mifrill
Mifrill marked this pull request as ready for review January 7, 2022 01:05
@MifrillMifrill added the needs feedback Needs feedback label Jan 7, 2022
@MifrillMifrill added the enhancement New feature or request label Jan 12, 2022
@Mifrill
Mifrillforce-pushed the issue_82/implement_wait_for_selector branch from f435ba9 to 3b09739CompareJanuary 15, 2022 10:18
@MifrillMifrill added ready for review and removed needs feedback Needs feedback labels Jan 15, 2022
@Mifrill
Mifrill requested a review from routeJanuary 15, 2022 10:22
Comment threadlib/ferrum/frame/dom.rb Outdated
end

def wait_for_selector(css: nil, xpath: nil, timeout: 3000, interval: 100)
evaluate_func(%(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be more consistent with the rest of the gem, would you mind changing this to use a JS heredoc? This is helpful because many editors support code highlighting in the language specified:

Original:
image

VS:

Subl:
image

Code:
image

I haven't tried the code out yet, but I'm looking forward to this feature!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

it makes sense, will change, thanks!

Comment threadlib/ferrum/frame/dom.rb Outdated
waitForSelector(resolve, reject);
});
}
), css || xpath, css.nil? && !xpath.nil?, timeout, interval, awaitPromise: true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was feeling like this arguments list was a little crowded, and also noticed this function could be incorrectly called without raising an exception: one of css or xpath is required, but not handled. What do you think about something like this:

defwait_for_selector(css: nil,xpath: nil,timeout: 3000,interval: 100)raiseArgumentError.new("Provide :css or :xpath, but not both")ifcss && xpathraiseArgumentError.new("Provide :css or :xpath")unlesscss || xpathevaluate_func(<<~JS,css || xpath, !!xpath,timeout,interval,awaitPromise: true) function(selector, isXpath, timeout, interval) { var attempts = 0;

The guard clauses protect the arguments from being called incorrectly, and also simplify the resolution to isXpath, since it can now only be one or the other.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

well, it looks like too much for one method to be responsible for selector/XPath and also for validations.
However, I see the confusing point here as well. In this case, I think we should use simple methods for more clear API, like those:

defwait_for_selector(css, **options)wait_for_element(css: css, **options)enddefwait_for_xpath(xpath, **options)wait_for_element(xpath: xpath, **options)end

The point to merge two ways of element-find logic was a follow a DRY principle for a common JS function, especially this part:

varelement=isXpath
? document.evaluate(selector,document,null,XPathResult.FIRST_ORDERED_NODE_TYPE,null).singleNodeValue
: document.querySelector(selector);

therefore, I think, we can find a way to refactor it with a private function by passing a js function, gimme see closer, will do an update soon.

@ttilbergttilbergFeb 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice one, yes, I think this is a better direction. Consider wait_for_css rather than wait_for_selector as a selector could generically mean css or xpath. It makes the expected arg more clear.

@dsisnero

Copy link
Copy Markdown

Can this be merged? Do the linters need to be changed or do we want to follow the linter advice?

@Mifrill

Copy link
Copy Markdown
ContributorAuthor

Hello @dsisnero this PR is on review, it can't be merged without approval.
The linter offenses are not related to this particular PR, we have dedicated PR to solve it:
#237 that also on review.

@route

Copy link
Copy Markdown
Member

I'm sorry guys, with all this stuff happening I barely find time for open source now, hope it's going to calm down soon.

@Nakilon

Nakilon commented Jun 18, 2022

Copy link
Copy Markdown
Contributor

If anyone needs a simple substitute of assert_selector or Selenium::WebDriver::Wait.new.until &block and google leads him here, here is a simple ruby snippet to wait for arbitrary things to return true:

timeout_true=lambdado |timeout=1, &block|
Timeout.timeouttimeoutdoblock.yieldor(sleep0.1; redo)endend

@shreeve

Copy link
Copy Markdown

Any chance we could have a reviewer review and accept this? It would be very nice to have in the main Gem.

Thanks!

@shreeve

shreeve commented Nov 23, 2022

Copy link
Copy Markdown

Would this be simpler?

classFerrum::Browserdefwait_for(want,wait:1,step:0.1)meth=want.start_with?('/') ? :at_xpath : :at_cssuntilnode=send(meth,want)(wait -= step) > 0 ? sleep(step) : breakendnodeendend

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement wait_for_selector

6 participants

@Mifrill@dsisnero@route@Nakilon@shreeve@ttilberg