Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed encoding mishaps. Adjusted tests. #499

Merged
merged 1 commit into from
Jun 4, 2014
Merged

Fixed encoding mishaps. Adjusted tests. #499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions lib/static.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ var load = exports.load = function(content, options) {

// Add in the root
initialize._root = root;
// store options
initialize._options = options;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows to pass options object to .html method.


return initialize;
};
Expand All @@ -37,11 +39,16 @@ var load = exports.load = function(content, options) {
*/

var html = exports.html = function(dom) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous this.options was always undefined, this is why render methods encoded output data no matter if it was decoded or not before.

var Cheerio = require('./cheerio');
// sometimes $.html() used without preloading html
// so fallback non existing options to the default ones
var options = _.defaults(this._options || {}, Cheerio.prototype.options);

if (dom) {
dom = (typeof dom === 'string') ? select(dom, this._root, this.options) : dom;
return render(dom, this.options);
dom = (typeof dom === 'string') ? select(dom, this._root, options) : dom;
return render(dom, options);
} else if (this._root && this._root.children) {
return render(this._root.children, this.options);
return render(this._root.children, options);
} else {
return '';
}
Expand Down
3 changes: 2 additions & 1 deletion test/cheerio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,11 @@ describe('cheerio', function() {

it('should render xml in html() when options.xmlMode = true', function() {
var str = '<MixedCaseTag UPPERCASEATTRIBUTE=""></MixedCaseTag>',
expected = '<MixedCaseTag UPPERCASEATTRIBUTE=""/>',
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope of this PR, but Cheerio#html won't pass any options, leading to unexpected behavior for $("MixedCaseTag").html().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I follow you on this one, does it mean you don't want .html() to respect xmlMode:true flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In contrary: Currently, Cheerio#html doesn't pass any options to $.html, meaning xmlMode will simply be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this test proves it. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not in the scope of this PR. I only wanted to point it out, to ensure it won't be missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be awesome; I already created #501 as a reference, but a patch would be very appreciated.

});

});