Explicit window vars in registerServiceWorker.js #2752
Conversation
The props `URL`, `fetch`, `location`, `addEventListener` are available under the global `window` object in the browser. This was previously unclear to linter plugins such as `standardjs`. The solution is to either prepend all of those variables with `window.*` OR destructure them from the `window` object as shown in this commit.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
|
||
| // To learn more about the benefits of this model, read https://goo.gl/KwvDNy. | ||
| // This link also includes instructions on opting out of this behavior. | ||
| const {URL, fetch, location, addEventListener} = window; |
There was a problem hiding this comment.
I would prefer we just explicitly keep referring to them as window.<whatever> rather than destructure here.
There was a problem hiding this comment.
Me too, at least window.location and window.addEventListener
There was a problem hiding this comment.
Thanks for the feedback. Made changes accordingly.
|
Thanks! Left a style nit but otherwise it's good to go. |
| function checkValidServiceWorker(swUrl) { | ||
| // Check if the service worker can be found. If it can't reload the page. | ||
| fetch(swUrl) | ||
| window.fetch(swUrl) |
There was a problem hiding this comment.
Hmm. I don't think we want to do this. Pretty much all tutorials use fetch without qualifier and I don't want to confuse readers.
I'm fine with other changes but I'd rather keep this one as is. If somebody uses Standard they can feel free to change it by hand in their project.
There was a problem hiding this comment.
a standard user also has the option of adding
"standard": { "globals": "fetch" }to their package.json
|
Closing as stale |
6 participants
The props
URL,fetch,location,addEventListenerare available under the globalwindowobject in the browser. This was previously unclear to linter plugins such asstandardjs. The solution is to either prepend all of those variables withwindow.*OR destructure them from thewindowobject as shown in this commit.Tested the code change locally and got no console or hot-reload errors.