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

root will be overwritten #1706

Closed
wants to merge 5 commits into from
Closed

root will be overwritten #1706

wants to merge 5 commits into from

Conversation

5saviahv
Copy link
Contributor

@5saviahv 5saviahv commented Feb 1, 2021

root will be overwritten if you parse new html content.

lets say you

const $ = cheerio.load(html);

const el = $('<div></div>').add('#fruits');
// should return 2 elements but instead you get just one

turns out since el._root & el are same instance. if you add div into dom you effectively overwriting root. Now when you plan make query against that old root you get empty results since new empty div wont have old content.

@5saviahv 5saviahv closed this Feb 3, 2021
@5saviahv 5saviahv deleted the saveroot branch February 3, 2021 07:45
lib/cheerio.js Outdated Show resolved Hide resolved
@fb55
Copy link
Member

fb55 commented Feb 4, 2021

@5saviahv This is yet another great PR. I have one question about this, otherwise it should have been pretty much good to go.

@5saviahv 5saviahv restored the saveroot branch February 4, 2021 20:39
@5saviahv 5saviahv reopened this Feb 4, 2021
@5saviahv
Copy link
Contributor Author

5saviahv commented Feb 4, 2021

My doubts were mainly if context is provided and is object - do I have to add some more testing there ?

Co-authored-by: Felix Böhm <me@feedic.com>
@5saviahv 5saviahv closed this Feb 11, 2021
@5saviahv 5saviahv deleted the saveroot branch February 24, 2021 23:53
@@ -53,7 +53,7 @@ var Cheerio = (module.exports = function (selector, context, root, options) {

if (root) {
if (typeof root === 'string') root = parse(root, this.options, false);
this._root = Cheerio.call(this, root);
this._root = new Cheerio(root);
Copy link
Member

Choose a reason for hiding this comment

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

Finally figured out what's wrong here: This should be new this.constructor(root). Incorporating this in a different PR.

fb55 added a commit that referenced this pull request May 14, 2021
Co-Authored-By: 5saviahv <49443574+5saviahv@users.noreply.github.com>
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

2 participants