-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Initial browserify support #332
Conversation
Simple fix to get around the conditional require for lib-cov.
Maybe the version should also be added to the regular package. So far, I don't see any real reasons for maintaing a separate file for it. |
My thought's exactly. Also: we shouldn't define a separate build until we have tests for it. |
@fb55 What do you mean by "added to the regular package"? Browserify has trouble with the conditional require of @jugglinmike I agree. I tried adding a test similar to var browserify = require('browserify');
var vm = require('vm');
var path = require('path');
var b = browserify();
b.require('../', { expose: 'cheerio' });
b.bundle(function(err, src) {
console.log(err);
var ctx = { console: console };
vm.runInNewContext(src, ctx);
console.log(ctx.require('cheerio'))
}); But browserify was having a really hard time loading the current module. |
I don't understand. By "the current module", do you mean Cheerio itself? And isn't that expected (given that the patches this depends on have not been published yet)? |
Yes, cheerio itself. Ideally it would test that it could browserify and execute some dummy cheerio code. I had symlinked in packages that compensated for the patches not having landed, and browserify was still failing to load cheerio itself. I could have been confused though, I was doing a lot of symlinking and every time I fixed one issue, another one cropped up! So far the biggest stopper is the one in node-entities where there are requires that map across several different strings. |
I see @kirbysayshi . I appreciate you doing the leg work for this, but at the moment, it doesn't sound like this pull request is suitable to merge. How would you feel about closing this until Cheerio could be built using browserify and that build could be tested? |
If anyone wants a quick and dirty browserified cheerio: https://gist.github.com/ganeshv/8349757 Haven't tested it all that much, but it's enough to meet my current requirements. I'm looking forward to the official version. Hopefully there is a way to get a browser-based version somewhat smaller than the current ~450KB. Cheerio/htmlparser2 is exactly what I was looking for - I want the ability to process HTML fragments with a standard JQuery-like API. I can't use JQuery itself because I don't want the side effects of browser-DOM-based parsing (which causes images to be loaded, for instance). |
[#66517740] Based on: https://gist.github.com/ganeshv/8349757#file-cheerio-js Also see discussion on Github: cheeriojs/cheerio#332
Initial stab at #268.
Depends on fb55/entities#12, as well as fb55/css-select#16.