Fixed encoding mishaps. Adjusted tests. #499
Conversation
alexindigo
changed the title
| @@ -28,6 +28,8 @@ var load = exports.load = function(content, options) { | |||
|
|
|||
| // Add in the root | |||
| initialize._root = root; | |||
| // store options | |||
| initialize._options = options; | |||
There was a problem hiding this comment.
This allows to pass options object to .html method.
|
Nice patch in general. Would you mind submitting the renderer patches to dom-serializer? Otherwise, #458 isn't mergeable. |
|
Done – cheeriojs/dom-serializer#4 Although I'm not sure if makes total sense, since half of the job was already done. |
|
@alexindigo Could you update this to fix the failing test case? |
|
Updated PR. |
|
PS. Btw, somewhat unrelated I noticed that in package.json it depends on [email protected], and at the moment CSSselect is at 0.7.0 version, is it intentionally left at older version? |
| dom = $.load(str, {xmlMode: true}); | ||
|
|
||
| expect(dom('MixedCaseTag').get(0).name).to.equal('MixedCaseTag'); | ||
| expect(dom.html()).to.be(str); | ||
| expect(dom.html()).to.be(expected); |
There was a problem hiding this comment.
Out of scope of this PR, but Cheerio#html won't pass any options, leading to unexpected behavior for $("MixedCaseTag").html().
There was a problem hiding this comment.
I'm not sure if I follow you on this one, does it mean you don't want .html() to respect xmlMode:true flag?
There was a problem hiding this comment.
In contrary: Currently, Cheerio#html doesn't pass any options to $.html, meaning xmlMode will simply be ignored.
There was a problem hiding this comment.
It does inherit options set on load:
(from lib/static.js)
var load = exports.load = function(content, options) {
// ...
// store options
initialize._options = options;
//...
}
var html = exports.html = function(dom) {
// ...
// sometimes $.html() used without preloading html
// so fallback non existing options to the default ones
var options = _.defaults(this._options || {}, Cheerio.prototype.options);
// ...
}
It was part of this PR.
There was a problem hiding this comment.
And this test proves it. :)
There was a problem hiding this comment.
I'm referring to lib/api/manipulation.js#L236:
return $.html(this[0].children);children is an array and doesn't have a _options property. As a result, no options will be passed.
There was a problem hiding this comment.
Yes, when there was no .load() prior html, then it's default options, won't be hard to add second argument to .html itself, everything is there.
But it this case we called .load first, so dom object has _options property.
// Yeah, it took me some time to figure out the right context with all that sticking "static" methods around. :)
There was a problem hiding this comment.
It's not in the scope of this PR. I only wanted to point it out, to ensure it won't be missed.
There was a problem hiding this comment.
Yep, it would require more thinking, since not all the options would make effect, like decodeEntities for example, but in general it seems like good thing to have, and won't be hard to implement, since it's all there already. I can submit another one with how I see it and we can continue discussion there.
There was a problem hiding this comment.
That would be awesome; I already created #501 as a reference, but a patch would be very appreciated.
Fixed encoding mishaps. Adjusted tests.
|
Thanks! |
|
Thank you, when we can expect fresh stuff in npm? :) |
|
Quotes inside <style> tags still seem to be getting quoted however. Example case where this is a problem is font-family, often you will find fonts quoted. |
|
Oh apologies, this is indeed fixed in the latest master. |
Fixed encoding problems I bumped into and saw over different issues here.
/cc #319 #460 #466 #496