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

rc.4 and rc.5 crash on a script that works in rc.3 #1679

Closed
ljharb opened this issue Jan 16, 2021 · 12 comments · Fixed by #1680
Closed

rc.4 and rc.5 crash on a script that works in rc.3 #1679

ljharb opened this issue Jan 16, 2021 · 12 comments · Fixed by #1680
Labels

Comments

@ljharb
Copy link

ljharb commented Jan 16, 2021

To repro:

  1. Clone https://github.com/ljharb/es-abstract/ and npm install
  2. npm install cheerio@1.0.0-rc.3
  3. node operations/getOps 2020, observe it completes
  4. npm install cheerio@1.0.0-rc.4 (or 5)
  5. node operations/getOps 2020, observe it crashes with:
$PWD/node_modules/cheerio/lib/api/traversing.js:30
    return newElems.concat(elem.children.filter(isTag));
                                         ^

TypeError: Cannot read property 'filter' of undefined
    at $PWD/node_modules/cheerio/lib/api/traversing.js:30:42
    at Array.reduce (<anonymous>)
    at module.exports.exports.find ($PWD/node_modules/cheerio/lib/api/traversing.js:29:30)
    at Object.<anonymous> ($PWD/operations/getOps.js:29:43)

Since this is a dev-only script, I only need to use it in node 14 or higher.

ljharb added a commit to ljharb/es-abstract that referenced this issue Jan 16, 2021
@5saviahv
Copy link
Contributor

5saviahv commented Jan 16, 2021

yes, it seems like in find function does not have condition determining if collection element is text node.

@5saviahv
Copy link
Contributor

5saviahv commented Jan 16, 2021

Cheerio should be able handle it, but I would suggest change code so document will be parsed with load function

...
const root = $.load(specHTML);

let aOps = root('[aoid]');

if (aOps.length === 0) {
	aOps = root('p:contains(" abstract operation ")').closest('section').add(root('#sec-reference-specification-type > section'));
}
...

@ljharb
Copy link
Author

ljharb commented Jan 16, 2021

What’s the advantage of that?

@5saviahv
Copy link
Contributor

This is how Cheerio is meant to work. Load document and then make requests against it.

Current setup also should work, but it is clearly not enough tested.

@ljharb
Copy link
Author

ljharb commented Jan 16, 2021

I expect cheerio to work just like jquery works :-) but thanks, appreciate the workaround.

@fb55
Copy link
Member

fb55 commented Jan 19, 2021

Thanks for the report! Quite unfortunate that this happened & thanks @5saviahv for stepping in!

@ljharb
Copy link
Author

ljharb commented Jan 19, 2021

Thanks, with this and hopefully #1585 fixed, i'll be able to update to rc6 (or a regular v1) everywhere ;-)

@lsbyerley
Copy link

lsbyerley commented Mar 21, 2021

I have come across this issue as well after upgrading from 1.0.0-rc.3 to 1.0.0-rc.5 .. however, i'm not exactly sure what I should be doing differently. My function is below and the error is generated at the .find() line

  const myFunc = async () => {
    const url = 'https://www.myurl.com';
    const theRes = await axios.get(url);

    const $ = cheerio.load(theRes.data, { normalizeWhitespace: true });
    const theDiv = $('#some_div_id');
    let rawHtml = theDiv.html();
    rawHtml = rawHtml.replace('<!--', '');
    rawHtml = rawHtml.replace('-->', '');

    const theDomNodes = cheerio.parseHTML(rawHtml);
    const theRows = $(theDomNodes).find('table#some_table_id tbody tr');

  ...
}

@fb55
Copy link
Member

fb55 commented Mar 21, 2021

Hi @lsbyerley, what error are you seeing?

@lsbyerley
Copy link

@fb55 the same one posted by the OP..

TypeError: Cannot read property 'filter' of undefined
at /sandbox/node_modules/cheerio/lib/api/traversing.js:30:42

@ljharb
Copy link
Author

ljharb commented Mar 21, 2021

The fix isn’t released yet, afaict

@lsbyerley
Copy link

alright no problem, i can downgrade until the release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

4 participants