-
Notifications
You must be signed in to change notification settings - Fork 662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chore: Move evaluate
to child process
#494
Conversation
|
||
try { | ||
const script: vm.Script = new vm.Script(source); | ||
const evaluteResult = await script.runInContext(jsdomutils.implForWrapper(window.document)._global); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After navigate you should wait for this._options.waitFor
as it does in jsdom.ts
. In that way, you will have a version of the site as much similar to the parent one as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarvaje Done.
src/lib/rules/axe/axe.ts
Outdated
@@ -11,7 +11,7 @@ import { AxeResults, Result as AxeResult, NodeResult as AxeNodeResult } from 'ax | |||
|
|||
import { readFileAsync } from '../../utils/misc'; | |||
import { debug as d } from '../../utils/debug'; | |||
import { IAsyncHTMLElement, ITraverseEnd, IRule, IRuleBuilder, Severity } from '../../types'; // eslint-disable-line no-unused-vars | |||
import { IAsyncHTMLElement, IRule, IRuleBuilder, Severity, ITraverseStart } from '../../types'; // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it, we have a problem switching to traverse::start
with chrome
.
We might want to create a custom event for evaluating scripts and each connector
can fire it when it is more appropriate.
Maybe something like canevaluate
?
@sonarwhal/contributors what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@molant I haven't encounter any issue with axe
using chrome
so far. What's the issue with using traverse::start
in chrome?
Now that I think about it, we have a problem switching to traverse::start with chrome.
That way maybe we can have a more clear message to show when evaluate is running, like Running scripts...
, rather than hanging at Traversing the DOM
when it's actually evaluating.
Maybe something like canevaluate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't encounter any issue with axe using chrome so far.
The problem is that we will be telling people to evaluate
scripts on traverse::start
. Those scripts could modify the DOM and alter the results. That's why I'd rather do all of that at the end of the process for chrome
. With jsdom
we don't have that problem because it is a separate process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@molant I put it back as traverse::end
for now before we decide whether or not we should have a custom event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, the discussion about this is now in #497
536d72f
to
2f34e68
Compare
2f34e68
to
cbb55cb
Compare
@alrra what do you think about the tagging of this PR? Maybe use a |
It's a user facing thing ("This solves the stuck spinner issue") so yes, use |
I'll update the commit message when squashing. There is a huge queue on travis an don't want to wait even more to merge this. |
evaluate
to its child processevaluate
ontraverse::start
This solves the stuck spinner issue.
Fix #485