Skip to content

Expose DOM level 1 API for Node-like objects#561

Merged
fb55 merged 2 commits into
cheeriojs:masterfrom
jugglinmike:dom-lvl-1-2
Sep 19, 2014
Merged

Expose DOM level 1 API for Node-like objects#561
fb55 merged 2 commits into
cheeriojs:masterfrom
jugglinmike:dom-lvl-1-2

Conversation

@jugglinmike

Copy link
Copy Markdown
Member

Now that Cheerio preserves parsing options, we can use the withDomLvl1 option in htmlparser2's default DomHandler (which we prototyped in gh-308) to expose a standard API for the Node-like objects that Cheerio builds.

I've updated the tests and documentation to demonstrate this usage, but I've left the internals untouched. Because the new API is defined via ES5 "getters", it will be slightly more efficient to use the original, non-standard API internally. If anyone thinks that this could cause more confusion than its worth (probably justified via our benchmarks), I'd be happy to update the internals as well.

Commit message:

Now that htmlparser2's default DomHandler supports the withDomLvl1
option, enable it by default for all parsed content.

Update the project documentation and tests to use this standard API to
exemplify usage.

Now that htmlparser2's default DomHandler supports the `withDomLvl1`
option, enable it by default for all parsed content.

Update the project documentation and tests to use this standard API to
exemplify usage.
@jugglinmike jugglinmike mentioned this pull request Sep 18, 2014
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same when pulling 62d03c4 on jugglinmike:dom-lvl-1-2 into 5b76f2c on cheeriojs:master.

@davidchambers

Copy link
Copy Markdown
Contributor

This is great! It warrants at least a minor version bump, of course.

@jugglinmike

Copy link
Copy Markdown
Member Author

No objection here. Just to be clear: I think a minor version is exactly appropriate (not "at least") because this change only adds API--all previously-existing properties on the Node-like remain.

@davidchambers

Copy link
Copy Markdown
Contributor

all previously-existing properties on the Node-like remain

Oh, I didn't realize this is the case. Thanks for correcting me.

Instead of implicitly relying on other unit tests to exhaustively assert
the "shape" of the Node-like object's API, define a single test to
ensure that the documented properties are available and functioning as
expected.
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same when pulling 8850120 on jugglinmike:dom-lvl-1-2 into 5b76f2c on cheeriojs:master.

@jugglinmike

Copy link
Copy Markdown
Member Author

@davidchambers No problem--glad I didn't come off as (too) pedantic there :P

Also: I just pushed up an additional commit that adds a single test for this API. As noted in the commit message, this explicltly ensures that all the documented attributes are functioning correctly (so we can be positive the documented API is available even if the other tests should change).

@davidchambers

Copy link
Copy Markdown
Contributor

LGTM

@jugglinmike

Copy link
Copy Markdown
Member Author

Thanks, @davidchambers

@matthewmueller @fb55 Would either of you mind looking this over an merging?

fb55 added a commit that referenced this pull request Sep 19, 2014
Expose DOM level 1 API for Node-like objects
@fb55 fb55 merged commit bb0c68a into cheeriojs:master Sep 19, 2014
@fb55

fb55 commented Sep 19, 2014

Copy link
Copy Markdown
Member

Thanks @jugglinmike!

@jugglinmike jugglinmike mentioned this pull request Sep 19, 2014
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants