Prevent TypeError due to undefined property when parsing styles #69
Conversation
| @@ -17,7 +17,10 @@ function parseStyles(s) { | |||
| parser.addListener("property", function(e) { | |||
| if (e.invalid) return; // Skip errors | |||
| result[e.property.text] = e.value.text; | |||
| if (e.important) result.important[e.property.text] = e.important; | |||
| if (e.important) { | |||
| result.important = {}; | |||
There was a problem hiding this comment.
Shouldn't this be:
result.important = result.important || {};to avoid rewriting the object in case it exists already?
| @@ -17,7 +17,10 @@ function parseStyles(s) { | |||
| parser.addListener("property", function(e) { | |||
| if (e.invalid) return; // Skip errors | |||
| result[e.property.text] = e.value.text; | |||
| if (e.important) result.important[e.property.text] = e.important; | |||
| if (e.important) { | |||
| result.important = result.important || {}; | |||
There was a problem hiding this comment.
Instead of doing this check every time, result is defined above. Why not do var result = { important: {} };?
Also, naive question, is there a chance e.property.text === 'important'? In which case, L19 may be problematic.
|
Could you add a test case for this as well? |
|
I updated this to incorporate the suggestion from @arlolra and to add a test in /test/domino.js that fails without and passes with the change. (@cscott, I may have misunderstood, were you asking in two places for one test, or two different tests?) @arlolra, going to your other question, I don't think there's a risk of "important" being used as a CSS property separate from its use as a priority rule... |
|
This is causing quite some exceptions in prod for the Mobile Content Service, so we'd appreciate a quick release. @mdholloway could you also bump the patch version of the pkg so that we can deploy it soon? |
mdholloway
force-pushed
the
master
branch
2 times, most recently
from
ec3e338
to
a510abb
Compare
|
Please don't bump the version in this patch. I have a release process for
that.
|
|
@cscott can we get a timeline for the next release / version bump? |
|
Tonight.
|
Fixes #68.