Skip to content
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

Fix: Unhandled promise rejections when self-signed in jsdom #3128

Merged
merged 3 commits into from Oct 18, 2019

Conversation

@sarvaje
Copy link
Member

sarvaje commented Oct 17, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Copy link
Member

antross left a comment

A couple of nits, but nothing blocking.

strictSSL: !options.ignoreHTTPSErrors
},
options.requestOptions || {}
);

This comment has been minimized.

Copy link
@antross

antross Oct 17, 2019

Member

How about using object-spread here?

const requesterOptions = {
    rejectUnauthorized: !options.ignoreHTTPSErrors,
    strictSSL: !options.ignoreHTTPSErrors,
    ...options.requestOptions
};
let html = '';

try {
const networkData = await requester.get(url);

This comment has been minimized.

Copy link
@antross

antross Oct 17, 2019

Member

Probably worth a comment here or before the run function explaining we need to request the HTML separately instead of just using JSDOM.fromURL in order to respect user options to ignore HTTPS errors.

return;
}

const jsdom = new JSDOM(html, {
beforeParse: beforeParse(url),
pretendToBeVisual: true,
resources: 'usable',

This comment has been minimized.

Copy link
@molant

molant Oct 17, 2019

Member

Shouldn't we use the resource loader you've created to make sure all the assets are downloaded? Otherwise CSS files and other scripts might not be downloaded. Not sure that will impact a lot the output of axe but just for being extra cautious.

This comment has been minimized.

Copy link
@sarvaje

sarvaje Oct 17, 2019

Author Member

sure, we can do that.

This comment has been minimized.

Copy link
@sarvaje

sarvaje Oct 17, 2019

Author Member

I add a custom resource loader, but that is can't be used for JSDOM.fromURL, because JSDOM.fromURL is expecting that the promise has a response property.

@sarvaje sarvaje requested review from molant and antross Oct 17, 2019
@molant
molant approved these changes Oct 18, 2019
@antross antross merged commit 81d5f84 into webhintio:master Oct 18, 2019
5 checks passed
5 checks passed
licence/cla Contributor License Agreement is signed.
Details
webhintio.hint Build #20191017.15 succeeded
Details
webhintio.hint (Linux_PR node_12_x) Linux_PR node_12_x succeeded
Details
webhintio.hint (Windows_PR node_12_x) Windows_PR node_12_x succeeded
Details
webhintio.hint (macOS_PR node_12_x) macOS_PR node_12_x succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.