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.

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

Parse html using $.parseHTML. #361

Closed
wants to merge 5 commits into from
Closed

Conversation

ssafejava
Copy link
Collaborator

See #351. In version 0.12.1, cheerio supports $.parseHTML. More on $.parseHTML from jQuery's tracker.

@jugglinmike, what do you think about wrapping the return value of $.parseHTML in cheerio inside $()? While all tests pass in the browser with an unwrapped $.parseHTML call, they fail in cheerio - I get the following output using console.log in the tests:

actual:
<div class="listContainer">List Container<div class="list">[object Object][object Object]</div></div>
expected:
<div class="listContainer">List Container<div class="list"><div class="item"><span class="title highlight">Item 1</span></div><div class="item"><span class="title highlight">Item 2</span></div></div></div>

@@ -927,6 +925,12 @@ LayoutManager.prototype.options = {
// Override this with a custom HTML method, passed a root element and content
// (a jQuery collection or a string) to replace the innerHTML with.
html: function($root, content) {
// Use $.parseHTML to safely convert strings into HTML. Cheerio requires
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that really necessary? Can a string end up here without that being a bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, strings would reach it through applyTemplate, if manager.noel was false. I fixed that in the next commit and removed $.parseHTML from options.html.

@SBoudrias
Copy link
Collaborator

I like the simplified version!

Although, I think the $.parseHTML bug should be adressed in Cheerio and eventually remove the fix here. (Although like you, I wonder if Mike have any insight on the behaviour - maybe this was a conscient choice for Cheerio folks)

@jugglinmike
Copy link
Collaborator

@ssafejava I wrote the same code yesterday after Cheerio 0.12.1 was published, but I felt that it's only exchanging one sub-optimal shim for another. @SBoudrias is correct--this is a bug in Cheerio. We might as well stick with what works for now until we have a complete solution.

As I mentioned earlier, I'm taking responsibility for #351. It's just going to take a little more time:

  1. Yesterday, I took steps to update the jQuery documentation..
  2. Once that lands, I'll fix the bug in Cheerio.
  3. Finally, we'll be able to simply use $.parseHTML consistently in both environments.

@ssafejava
Copy link
Collaborator Author

Thanks for handling it @jugglinmike! I can leave the PR open and we can just patch it to remove the $() wrapper around $.parseHTML when Cheerio is ready.
On Aug 1, 2013, at 8:25 PM, jugglinmike notifications@github.com wrote:

@ssafejava I wrote the same code yesterday after Cheerio 0.12.1 was published, but I felt that it's only exchanging one sub-optimal shim for another. @SBoudrias is correct--this is a bug in Cheerio. We might as well stick with what works for now until we have a complete solution.

As I mentioned earlier, I'm taking responsibility for #351. It's just going to take a little more time:

Yesterday, I took steps to update the jQuery documentation..
Once that lands, I'll fix the bug in Cheerio.
Finally, we'll be able to simply use $.parseHTML consistently in both environments.

Reply to this email directly or view it on GitHub.

@tbranyen
Copy link
Owner

tbranyen commented Sep 3, 2013

@ssafejava yup, that's what we're gonna do. We got the jQuery API docs updated (live now) and are progressing with the next step in keeping the Cheerio API matching closer.

@tbranyen
Copy link
Owner

tbranyen commented Sep 5, 2013

I removed the wrapper and updated the test's intention to not be focused on trimming, but instead the retaining source via parseHTML.

We're going to merge this as soon as Cheerio releases a version bump with @jugglinmike's fixes.

@tbranyen
Copy link
Owner

tbranyen commented Sep 5, 2013

Closed in #380

@tbranyen tbranyen closed this Sep 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants