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

remove jsdom dep & install jsdom in benchmark #853

Closed
wants to merge 1 commit into from
Closed

remove jsdom dep & install jsdom in benchmark #853

wants to merge 1 commit into from

Conversation

sartrey
Copy link

@sartrey sartrey commented Apr 27, 2016

I'm trying to remove useless dep:jsdom
Please review my changes ~

@coveralls
Copy link

coveralls commented Apr 27, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 98.824% when pulling 694b973 on sartrey:remove-jsdom-dep into 7b59afb on cheeriojs:master.

@sartrey sartrey mentioned this pull request Apr 27, 2016
Closed
@fb55
Copy link
Member

fb55 commented Apr 27, 2016

The reason for moving jsdom to optionalDependencies was its dependency contextify, which didn't compile on all node versions. Recent versions of jsdom don't require contextify, so it can be moved back to devDependencies (and the version can be increased to ^8.4.0).

@sartrey
Copy link
Author

sartrey commented Apr 28, 2016

Fine! I'll close this pr and let's use devDependencies instead ~
Any plan to do this in a fix version, like 0.20.1?
I could hardly wait to use smaller cheerio ...

@sartrey sartrey closed this Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sartrey @coveralls @fb55