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

throw a useful error on invalid input to cheerio.load() #1087

Merged
merged 5 commits into from
Mar 16, 2018

Conversation

zeke
Copy link
Contributor

@zeke zeke commented Sep 30, 2017

👋 @jugglinmike

This PR updates cheerio.load() to throw a more useful error message when bad input is given.

$ npm i -g trymodule && trymodule cheerio
Package 'cheerio' was loaded and assigned to 'cheerio' in the current scope
REPL started...

> cheerio.load(null)
TypeError: Cannot read property 'parent' of null
    at Function.exports.update (/Users/z/.trymodule/node_modules/cheerio/lib/parse.js:69:25)
    at module.exports (/Users/z/.trymodule/node_modules/cheerio/lib/parse.js:19:11)
    at Function.exports.load (/Users/z/.trymodule/node_modules/cheerio/lib/static.js:27:14)
    at repl:1:9
    at ContextifyScript.Script.runInContext (vm.js:53:29)
    at REPLServer.defaultEval (repl.js:241:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:440:10)
    at emitOne (events.js:120:20)

lib/static.js Outdated
@@ -15,6 +15,10 @@ var serialize = require('dom-serializer'),
*/

exports.load = function(content, options) {
if (content === null || content === undefined) {
throw new TypeError('cheerio.load() expects a string');
Copy link
Member

Choose a reason for hiding this comment

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

Please throw a plain error.

@zeke
Copy link
Contributor Author

zeke commented Dec 26, 2017

@fb55 I changed TypeError to Error. This is ready for review again. 👀

@fb55
Copy link
Member

fb55 commented Dec 26, 2017

Awesome, thanks a lot! Will merge once tests pass.

expect(function() {
cheerio.load();
}).to.throwException(function(err) {
expect(err).to.be.am(Error);
Copy link
Member

Choose a reason for hiding this comment

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

Typo: an

@fb55 fb55 changed the base branch from master to v1.0.0 March 16, 2018 11:25
@fb55 fb55 merged commit 71d7afb into cheeriojs:v1.0.0 Mar 16, 2018
@zeke zeke deleted the throw-useful-error-on-null-input branch March 16, 2018 16:39
@zeke
Copy link
Contributor Author

zeke commented Mar 16, 2018

Thanks, @fb55!

fb55 added a commit that referenced this pull request Oct 3, 2018
* Use parse5 as a default parser (closes #863)

* Use documents via $.load

* Add test for #997

* Change options format

* Update unit test

Update test phrasing according to recent changes in parsing logic.

* 1.0.0-rc.1

* Improve release process

Limit responsibility of "pre-publish" script to simply validate the
project's `History.md` file (by verifying an entry for the current
release). Define a separate script for history generation. Separating
the workflow in this way allows for manual modification of the release
notes.

* Correct errors in Readme.md

* Document advanced usage with htmlparser2

* Update History.md (and include migration guide)

* Remove documentation for `xmlMode` option

Simply expose an option named `xml` that enables XML parsing via
htmlparser2 with the ability to specify additional options for that
parser.

* Rename `useHtmlParser2` option

This flag is used to control parsing behavior internally, but it is not
intended for use by consumers. Prefix the name with an underscore in
order to discourage users from relying on it.

* Re-write migration guide for version 1.0 (#1078)

Incorporate recent feedback from consumers who have experimented with
the version 1.0 release candidate.

* Pass locationInfo option to parse5 (#1155)

* Update css-select to the latest version 🚀 (#1158)

Breaking change: Invalid selectors now throw Errors, not SyntaxErrors.

* Use eslint & prettier, add precommit hook (#1152)

* chore(package): update mocha to version 5.0.4 (#1088)

* Ensure text nodes expose the DOM level 1 API

Since enabling the `withDomLvl1` parsing option, nodes cannot be created
with an object literal. Create new text nodes using the `evaluate`
function to ensure they expose the correct attributes.

* fixing missing prop(‘outerHTML’) implementation.
Added an ‘outerHTML’ case to the switch in the prop function, which wraps a clone of  in a container element, and sets  to that container's HTML (#945)

* Do not lint files excluded from version control (#1162)

This includes code coverage reports as generated by the command `make
test-cov`.

* Correct typo in git hook configuration (#1163)

* Correct typo in git hook configuration

* Reformat package manifest to satisfy linter

* Fix .text with a function as the argument

* Fix `.text` being called on a collection with size > 1 with a function

* chore(package): update coveralls to version 3.0.0 (#1086)

* Update jsdom to the latest version 🚀 (#1008)

* Throw a useful error on invalid input to cheerio.load() (#1087)

* Procedurally generate API documentation from source (#1165)

* Use parse5 to serialize the DOM, use lodash to clone dom

* Fix DoS/RCE vulnerability in [email protected] (#1179)

fixes #1175

*  Add eslint-plugin-jsdoc, improve documentation (#1168)

* Improve variable names (#1183)

Promote consistency in variable names within the project's source and
unit tests. This helps to highlight the distinction between the object
exported by the module and the function produced by the `load` method.
The latter value is expected to mimic the jQuery API, while the former
value generally should only be used for a small set of methods specific
to Cheerio:

- `load`
- `html`
- `xml`
- `text`

Other usages of the exported object are discouraged, and a future patch
will update the unit tests to reflect the usages that are endorsed for
long-term stability.

* Formally test deprecated APIs (#1184)

Methods named `load`, `html`, `xml`, and `text` are defined in many
locations:

Today, Cheerio defines multiple versions of methods named `load`,
`html`, `xml`, `text`, and `parseHTML`. These alternate versions may be
defined in up to three distinct parts of the API:

- exported by the Cheerio module
- as static methods of the "loaded" Cheerio factory function
- as instance methods of the "loaded" Cheerio factory function

Some of these are surperfluous, and because some unecessarily conflict
with idiomatic jQuery coding patterns, they have been designated for
future removal [1].

Add tests for the deprecated methods in order to avoid regressions prior
to their removal. Insert comments to delineate the methods which are
endorsed and which have been deprecated. For the latter group of
methods, include recommendation for the preferred alternatives.

[1] #1122

* Implement for...of iterator via Symbol.iterator (#1197)

* Implement for...of iterator via Symbol.iterator

Similar to jQuery: https://github.com/jquery/jquery/blob/1ea092a54b00aa4d902f4e22ada3854d195d4a18/src/core.js#L371-L373

Fixes #1191

* Assert that the iterator ends

#1197 (comment)
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.

None yet

2 participants