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

Make the `sink` module an optional feature. #128

Closed
wants to merge 1 commit into from

Conversation

@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 25, 2015

Consumers who provide their own sinks (like Servo) probably don't need this module. Disabling it reduces compile time by about 20% (a couple of seconds on my laptop). Probably reduces final link time and possibly binary size too, but I haven't measured those.

r? @kmcallister

@kmcallister
Copy link
Contributor

kmcallister commented Apr 25, 2015

Maybe they should move to a separate crate, even a separate repo. We just need that crate as a dev-dependency to build the examples and test suite.

@Ygg01
Copy link
Contributor

Ygg01 commented Apr 25, 2015

👍 to separate crate/repo.

Modularize all the things

@mbrubeck mbrubeck force-pushed the mbrubeck:sink-feature branch from 3aacd04 to a97a35d Apr 27, 2015
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Apr 27, 2015

Updated to move html5ever::sink into a separate html5ever_dom_sink crate. Bikeshedding of the crate name is welcome.

Besides moving the code, the only other change was turning RcDom::Handle into a newtype struct, to satisfy the coherence rules on impl Serializable for Handle.

@mbrubeck mbrubeck force-pushed the mbrubeck:sink-feature branch from a97a35d to a57189a Apr 27, 2015
Consumers who provide their own sinks (like Servo) probably don't need this
module.  Removing it reduces compile time by about 20% (a couple of seconds
on my laptop).
@kmcallister
Copy link
Contributor

kmcallister commented May 8, 2015

Merged, thanks!

@kmcallister kmcallister closed this May 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.