Fixed potential error where head does not exist in doc #273
Conversation
index.js
Outdated
| @@ -334,7 +334,7 @@ const inlineCss = async opt => { | |||
| style = document.createElement("style"); | |||
| style.type = "text/css"; | |||
| style.appendChild(document.createTextNode(allCss)); | |||
| head.appendChild(style); | |||
| head && head.appendChild(style); | |||
There was a problem hiding this comment.
this will hide potential error. I would prefer visible error instead of hiding
There was a problem hiding this comment.
Maybe a console.warning instead? or we could just append it to the end of the document rather than the head if the head isnt there?
There was a problem hiding this comment.
if the head isn't there it will not work as expected, where shall we insert those instead? We can't just ignore it, the whole thing could fail. What we can do is to create more user friendly error, like "Hey, we use head to do this manipulation, but we failed to find one, so tool doesn't work. Please add head"
There was a problem hiding this comment.
@stereobooster the thing now is it fails regardless of whether or not there are any stylesheets to even add. Also if the <style> is in the body, it will still work the same as it would in the head
There was a problem hiding this comment.
May I ask why there is no head in the first place?
There was a problem hiding this comment.
@stereobooster Im not asking to optimize for this specific case, I'm offering to fix a bug where a null reference exception occurs. Null reference exceptions in libraries are never features
There was a problem hiding this comment.
yes, but this fix also can hide error for other case. Fix would be to show human readable error
There was a problem hiding this comment.
@stereobooster Sure, but also just the fact that adding a style element when there are no linked stylesheets is not necessary, is it?
There was a problem hiding this comment.
Sure, but also just the fact that adding a style element when there are no linked stylesheets is not necessary, is it?
For sure, but you can check exactly this case if (criticalCss.length === 0) { return; }.
There was a problem hiding this comment.
I will update my PR in the next few days
|
TODO Summary:
|
|
@stereobooster Updated, still in the process of testing this.. Edit: Confirmed that this fixes the null reference error |
|
Code is cleaner now. Can you please add test-case for this? |
|
@stereobooster added a test |
|
Looks ok. I need some time to actually merge it and release |
| const head = document.head || document.getElementsByTagName("head")[0], | ||
| style = document.createElement("style"); | ||
| style.type = "text/css"; | ||
| style.appendChild(document.createTextNode(allCss)); | ||
|
|
||
| if (!head) | ||
| throw "No <head> element found in document"; |
There was a problem hiding this comment.
| throw "No <head> element found in document"; | |
| throw new Error("No <head> element found in document"); |
|
There is small thing to change, but I can do it. Thanks |
This issue occurred when I was doing some transformations to certain pages to generate partial static HTML snippits for inclusion via ajax; react-snap would fail because the element doesnt exist