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 some cross-browser compatibility issues #1593

Merged
merged 3 commits into from
Feb 2, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
transform: translate(0px, 5px);
border: none;
flex: 0 0 auto;
vertical-align: top;
background-repeat: no-repeat;
background-position: center center;
background-size: 16px 16px;
Expand Down
50 changes: 31 additions & 19 deletions lighthouse-cli/performance-experiment/report/scripts/perf-x.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ class ConfigPanel {
this._messageField = this._configPanel.querySelector('.js-message');
this._urlBlockingList = this._configPanel.querySelector('.js-url-blocking-patterns');
this._urlBlockingStatus = {};
this._reportId = new URL(window.location).searchParams.get('id');
const match = location.search.match(/[?&]id=([^&]*)/);
Copy link
Contributor

Choose a reason for hiding this comment

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

I use a polyfill in Viewer for URLSearchParams: https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-viewer/package.json#L33

Could you use that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your advice. Shall I move the dependencies from viewer to core? Or just add some dependencies to cli?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to keep the deps for the viewer since it's a separate app.

Since you've already done the work of moving over to the fallbacks, there's not a lot of benefit to adding polyfills for two small APIs. I'm fine with what you've got.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment here? // Compat: URL.searchParams isn't yet supported by _______
so later when that browser updates we'll know when we can remove it

this._reportId = match ? match[1]: '';

const bodyToggle = this._configPanel.querySelector('.js-panel-toggle');
bodyToggle.addEventListener('click', () => this._toggleBody());
Expand Down Expand Up @@ -83,15 +84,6 @@ class ConfigPanel {
}
});
});

// get and recover blocked URL patterns of current run
fetch(`/flags?id=${this._reportId}`).then(response => {
return response.json();
}).then(flags => {
const blockedUrlPatterns = flags.blockedUrlPatterns || [];
blockedUrlPatterns.forEach(urlPattern => this.addBlockedUrlPattern(urlPattern));
this.log('');
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But Safari 10.1 also has fetch(). A polyfill might not be worth it.

Copy link
Member

Choose a reason for hiding this comment

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

lets not polyfill fetch()

/**
Expand All @@ -100,15 +92,24 @@ class ConfigPanel {
*/
_rerunLighthouse() {
this.log('Start Rerunning Lighthouse');

const options = {
blockedUrlPatterns: this.getBlockedUrlPatterns()
};

return fetch(`/rerun?id=${this._reportId}`, {method: 'POST', body: JSON.stringify(options)})
.then(response => response.text())
.then(newReportId => location.assign(`?id=${newReportId}`))
.catch(err => this.log(`Lighthouse Runtime Error: ${err}`));
const xhr = new XMLHttpRequest();
xhr.open('POST', `/rerun?id=${this._reportId}`, true);
xhr.onreadystatechange = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

just use xhr.onload, then you don't need to both with readyState checking.

if (xhr.readyState !== 4) {
return;
}

if (xhr.status === 200) {
location.assign(`?id=${xhr.responseText}`);
} else if (xhr.status === 500) {
this.log('Error: Lighthouse runtime error');
}
};
xhr.send(JSON.stringify(options));
}

/**
Expand All @@ -118,8 +119,12 @@ class ConfigPanel {
* @param {string} urlPattern
*/
addBlockedUrlPattern(urlPattern) {
if (this._urlBlockingStatus[urlPattern]) {
this.log(`${urlPattern} is already in the list`);
urlPattern = urlPattern.trim();
if (urlPattern.length === 0) {
this.log('Error: URL blocking pattern is an empty string');
return;
} else if (this._urlBlockingStatus[urlPattern]) {
this.log(`Error: ${urlPattern} is already in the list`);
return;
}

Expand Down Expand Up @@ -150,8 +155,9 @@ class ConfigPanel {
* @param {string} urlPattern
*/
removeBlockedUrlPattern(urlPattern) {
urlPattern = urlPattern.trim();
if (!this._urlBlockingStatus[urlPattern]) {
this.log(`${urlPattern} is not in the list`);
this.log(`Error: ${urlPattern} is not in the list`);
return;
}

Expand Down Expand Up @@ -193,5 +199,11 @@ class ConfigPanel {
}

window.addEventListener('DOMContentLoaded', () => {
new ConfigPanel();
const configPanel = new ConfigPanel();

// init recover blocked URL patterns
[...document.querySelectorAll('.js-currently-blocked-url')].forEach(ele => {
configPanel.addBlockedUrlPattern(ele.getAttribute('data-url-pattern'));
});
configPanel.log('');
});
16 changes: 4 additions & 12 deletions lighthouse-cli/performance-experiment/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
* Browser can request lighthousr rerun by sending POST request to URL: /rerun?id=[REPORT_ID]
* This will rerun lighthouse with additional cli-flags received from POST request data and
* return the new report id
* Flags can be access via URL: /flags?id=[REPORT_ID]
*/

const http = require('http');
Expand Down Expand Up @@ -69,8 +68,6 @@ function requestHandler(request, response) {
if (request.method === 'GET') {
if (pathname === '/') {
reportRequestHandler(request, response);
} else if (pathname === '/flags') {
flagsRequestHandler(request, response);
} else {
throw new HTTPError(404);
}
Expand Down Expand Up @@ -118,15 +115,6 @@ function reportRequestHandler(request, response) {
}
}

function flagsRequestHandler(request, response) {
try {
response.writeHead(200, {'Content-Type': 'text/json'});
response.end(JSON.stringify(database.getFlags(request.parsedUrl.query.id || fallbackReportId)));
} catch (err) {
throw new HTTPError(404);
}
}

function rerunRequestHandler(request, response) {
try {
const flags = database.getFlags(request.parsedUrl.query.id || fallbackReportId);
Expand All @@ -142,6 +130,10 @@ function rerunRequestHandler(request, response) {
const id = database.saveData(flags, results);
response.writeHead(200);
response.end(id);
}).catch(err => {
log.error('PerformanceXServer', err.code, err);
response.writeHead(500);
response.end(http.STATUS_CODES[500]);
});
});
} catch (err) {
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/report/styles/report.css
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ body {
display: none;
padding: 10px 0 10px 42px;
top: 100%;
left: 0;
position: absolute;
width: 100%;
background: inherit;
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/report/templates/report-template.html
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ <h2 class="config-section__title">Runtime Environment</h2>
<h2 class="config-section__title">Blocked URL Patterns</h2>
<ul class="config-section__items">
{{#each runtimeConfig.blockedUrlPatterns }}
<li class="config-section__item">
<li class="config-section__item js-currently-blocked-url" data-url-pattern="{{ this }}">
<p class="config-section__desc">{{ this }}</p>
</li>
{{/each}}
Expand Down