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

Cheerio breaks when bundling with Rollup #1689

Closed
hansottowirtz opened this issue Jan 20, 2021 · 5 comments
Closed

Cheerio breaks when bundling with Rollup #1689

hansottowirtz opened this issue Jan 20, 2021 · 5 comments

Comments

@hansottowirtz
Copy link

Due to a circular dependency Rollup emits warnings when bundling Cheerio, and the resulting code is also broken. This wasn't possible in 1.0.0-rc.3 either, but there the problem was in readable-stream and htmlparser2, which could then be included as external dependencies. In 1.0.0-rc.5 the problem is in Cheerio.

This is caused by require('./cheerio'); in lib/static.js#L31
Somehow this causes static.js to be included after attributes.js, which relies on static.js when doing var text = require('../static.js').text in lib/api/attributes.js#L8.

Ideally this would be solved by refactoring Cheerio in ESM, but just removing the circular dependency in static.js would also work.

I made a repro here: hansottowirtz/cheerio-bundling-repro.

(!) Circular dependencies
node_modules/cheerio/lib/static.js -> node_modules/cheerio/lib/cheerio.js -> node_modules/cheerio/lib/api/attributes.js -> node_modules/cheerio/lib/static.js
node_modules/cheerio/lib/static.js -> node_modules/cheerio/lib/cheerio.js -> node_modules/cheerio/lib/api/attributes.js -> <project>/node_modules/cheerio/lib/static.
js?commonjs-proxy -> node_modules/cheerio/lib/static.js
node_modules/cheerio/lib/static.js -> node_modules/cheerio/lib/cheerio.js -> node_modules/cheerio/lib/api/manipulation.js -> node_modules/cheerio/lib/static.js
@5saviahv
Copy link
Contributor

5saviahv commented Jan 20, 2021 •
edited
Loading

Hmm, yes this require in function .load() needs fully built Cheerio object. It uses basically node module caching ability and this is why require are inside that function and not top level.

@5saviahv
Copy link
Contributor

5saviahv commented Jan 20, 2021 •
edited
Loading

I managed to use provided cheerio rollup test, but If I test that generated file it fails. So I have no idea, is my modified cheerio build failing or is rollup configuration flawed.

My build does work with all Jest tests so I think configuration, but I am not used rollup before so I am not sure.

@hansottowirtz
Copy link
Author

but If I test that generated file it fails

Yes, I made this test to show that the bundling doesn't work. If we could remove the circular dependency the test would not fail.

The project would need to be restructured slightly, because Rollup cannot rely on the node module caching ability. This would also be a problem when rewriting Cheerio using ESM, as there (ideally) can be no dynamic require.

So the question is how to restructure so there are no circular dependencies.

@hansottowirtz hansottowirtz mentioned this issue Jan 20, 2021
Closed
@5saviahv
Copy link
Contributor

I modified src/index.js in test setup

const cheerio = require('cheerio');

const $ = cheerio.load('<html></html>')

console.log($.html());

and it worked

@5saviahv 5saviahv mentioned this issue Jan 20, 2021
Merged
@fb55 fb55 closed this as completed in a9d6a43 Jan 25, 2021
@stephancasas
Copy link

Superb work on this one. I've included cheerio as a dependency using github: protocol with the commit number. Everything works great now. 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@hansottowirtz @stephancasas @5saviahv