Fix some cross-browser compatibility issues #1593
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
weiwei-lin
force-pushed
the
fix-cross-browser
branch
from
76d826b
to
b846c04
Compare
|
CLAs look good, thanks! |
| @@ -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=([^&]*)/); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
thanks for your advice. Shall I move the dependencies from viewer to core? Or just add some dependencies to cli?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -83,15 +84,6 @@ class ConfigPanel { | |||
| } | |||
| }); | |||
| }); | |||
|
|
|||
| // get and recover blocked URL patterns of current run | |||
| fetch(`/flags?id=${this._reportId}`).then(response => { | |||
There was a problem hiding this comment.
you could also use a polyfill for this:
https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-viewer/package.json#L34
There was a problem hiding this comment.
But Safari 10.1 also has fetch(). A polyfill might not be worth it.
| .catch(err => this.log(`Lighthouse Runtime Error: ${err}`)); | ||
| const xhr = new XMLHttpRequest(); | ||
| xhr.open('POST', `/rerun?id=${this._reportId}`, true); | ||
| xhr.onreadystatechange = () => { |
There was a problem hiding this comment.
just use xhr.onload, then you don't need to both with readyState checking.
| @@ -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=([^&]*)/); | |||
There was a problem hiding this comment.
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
In interactive mode, the report page will be opened in the default browser. However, the interactive mode report page only works in Chrome currently.
This PR adds support for Safari and Firefox.