TimBeyer / html-to-vdom

Converts an HTML string into a virtual DOM
172 stars 41 forks source link

Promises removed to allow gulp streaming. #6

Closed mattferrin closed 9 years ago

mattferrin commented 9 years ago

I don't know if you depend on promises, but they were chained deeply and prevented Gulp streaming. I'd suggest wrapping the API in a optional promise instead.

review-ninja commented 9 years ago

ReviewNinja

TimBeyer commented 9 years ago

Hey @mattferrin, thank you for your PR.
Since htmlparser has a callback based API I was under the impression that it's actually an asynchronous process, so I wrapped it in promises. Is it guaranteed for the parsing process to be sync now and in the future? In that case I'll gladly merge the PR. But then I am still wondering why htmlparser chose such a weird API.

mattferrin commented 9 years ago

@TimBeyer It's hard to see the code as a whole, but it appears that parseComplete completes a write synchronously and fully.

    Parser.prototype.parseComplete = function Parser$parseComplete (data) {
        this.reset();
        this.parseChunk(data);
        this.done();
    };

    Parser.prototype.done = function Parser$done () {
        this._state.done = true;
        this._parse(this._state);
        this._flushWrite();
        this._builder.done();
    };

See here's a plain for loop:

    Parser.prototype._flushWrite = function Parser$_flushWrite () {
        if (this._state.pendingWrite) {
            for (var i = 0, len = this._state.pendingWrite.length; i < len; i++) {
                var node = this._state.pendingWrite[i];
                this._builder.write(node);
            }
            this._state.pendingWrite = null;
        }
    };

But I'm very very sleep deprived :)

TimBeyer commented 9 years ago

@mattferrin thanks a lot for the PR. I cherry picked your commits, and added some other changes - most noticeably the switch to htmlparser2 - so I'll close this PR. I also added a new 'Credits' section for you to the README.md.

The updated version will be pushed to npm soon. I really hope I don't break the code for anyone who wasn't restrictive enough with semver. :pray:

TimBeyer commented 9 years ago

You might also be happy to hear that I just released a performance-tuned version of the library. You can see the comparison between the old and new version on http://jsperf.com/html-to-vdom-htmlparser-vs-htmlparser2

'htmlparser' is the old htmlparser based one 'htmlparser2' is the 0.2.0 release 'htmlparser2-betterperf' is 0.2.0 with improved attribute copying 'htmlparser2-betterperf2' is 0.2.1 which also includes improved dataset conversion

mattferrin commented 9 years ago

@TimBeyer Thank you for the great module. Using SVG icons as Browserify modules feels awesome :) I'll make sure to leverage and credit your contributions in both Gulp plugins when I get a chance to clean them up.