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

Correct bug when checking for ids with dots #32

Closed
wants to merge 1 commit into from
Closed

Correct bug when checking for ids with dots #32

wants to merge 1 commit into from

Conversation

OXINARF
Copy link

@OXINARF OXINARF commented Feb 3, 2016

If an id has dots (for example fig4.5.1) then cheerio doesn't work correctly. This makes the crawler fail although the anchor does exist.

@@ -68,7 +68,7 @@ module.exports = function (grunt) {

if (queueItem.url.indexOf('#') !== -1) {
try {
if ($(queueItem.url.slice(queueItem.url.indexOf('#'))).length === 0) {
if ($('[id="' + queueItem.url.slice(queueItem.url.indexOf('#')+1) + '"]').length === 0) {
Copy link

Choose a reason for hiding this comment

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

$('#' + queueItem.url.slice(queueItem.url.indexOf('#') + 1)) cheerio selectors are supposed to work the same way as jQuery (sizzlejs)

Copy link
Author

Choose a reason for hiding this comment

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

Your solution is what was there before ;)

Your statement is true but apparently in jQuery ids with dots don't work either - you can follow the discussion here: cheeriojs/cheerio#115

Copy link

Choose a reason for hiding this comment

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

I think this might be a bug further upstream in cheerio or css-select, we should investigate there, if we can't find a proper solution upstream we can carry on with this fix

Copy link
Author

Choose a reason for hiding this comment

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

I can't say if it isn't or not, I only know that it was reported to cheeiro and the developers where there and said it worked like it was supposed.

@greggman
Copy link
Contributor

Oh, haha. Doh! I just posted a fix for the same issue. I suppose I should have checked. Maybe you like the other PR

@OXINARF
Copy link
Author

OXINARF commented Mar 14, 2016

@greggman I like that your PR adds the dot tests, but your way to fix it isn't the appropriate way according to the discussion in cheerio that I already posted above (cheeriojs/cheerio#115)

@greggman
Copy link
Contributor

Sorry, I'm not understanding why my PR doesn't work according to that discussion. Escaping the periods works in jquery, in querySelector, and in cheerio. But in any case whatever fixes the issue would be great

@OXINARF
Copy link
Author

OXINARF commented Mar 14, 2016

It does work, but it seems to be because of the way an internal library works (which means that a change to the library can break your fix): cheeriojs/cheerio#115 (comment) If you read all the discussion the advised way is to use something like [id="foo.bar"], which is what my fix does.

Anyway, this repo doesn't seem to be updated anymore...

@OXINARF
Copy link
Author

OXINARF commented Jun 25, 2016

So the PR with the wrong way to fix the problem was merged, without even a word in this previous one.

Thanks @ChrisWren! I won't certainly open any other PR on anything you're involved.

@OXINARF OXINARF closed this Jun 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants