Should serde support be optional?

[vorner]

The serde support is not optional though someone might not need it. Serde (and especially serde-derive) is somewhat heavy dependency and it is quite common to have serde as a feature flag on many crates.

Would it be OK if I add the support?

[achanda]

My original reasoning was that serialization/deserialization support will be needed in a lot of networking code. But I guess some people might just want to iterate over networks and check a few attributes. If we feature flag serde, anyone who uses this as a dependency will have to opt in. That does look like a bit of a cognitive burden to me. Is there a benefit with respect to compile-time or binary size? For reference, this PR removed the feature flag https://github.com/achanda/ipnetwork/pull/74

[vorner]

There's also the option to have a feature flag, but default-on. Users don't need to opt in, but it's possible to opt out. Would that work?

There definitely may be compile-time benefit ‒ serde-derive depends on syn and quote and these are known compile-time hogs. Now, if you have a bigger project, the chance is you need them anyway for something else so for bigger projects this'll not be much different, but for little one-off hacks the difference might be something like 5s vs .5s.

I would not expect the binary size to be affected, I think functions that are not used will get eliminated, at least if lto is enabled.

There's another factor to unneeded dependencies. There are situations where you might want to review the code of all your dependencies (security, etc). Reviewing IpNetwork itself is quite fine. But if you have to throw the whole serde, serde-derive, quote, syn into the package then you're just going to reimplement the ip network stuff.

[faern]

Sorry, I did not even see this issue before I submitted #109 to make serde optional. I made it opt-out to not make a breaking change. But I think the more common pattern is to make it opt-in.

It's common that crates that optionally support serialization has an optional serde feature, so anyone familiar with that will already know it. And if we think the feature is undiscoverable I think it's nicer to have it opt-in but described in the documentation/readme than having it activated by default. Anyway, opt-out is way better than the dependency being mandatory.

[faern]

There is definitely compile time differences. Here tried with the branch in my PR:

$ cargo clean && time cargo build --release --no-default-features
   Compiling ipnetwork v0.15.1 (/home/user/src/ipnetwork)
    Finished release [optimized] target(s) in 0.93s

real    0m0.939s
user    0m1.214s
sys 0m0.106s

$ cargo clean && time cargo build --release
   Compiling serde v1.0.103
   Compiling ipnetwork v0.15.1 (/home/user/src/ipnetwork)
    Finished release [optimized] target(s) in 15.74s

real    0m15.753s
user    0m16.656s
sys 0m0.357s

So, roughly a factor 15x in compile time :)

[achanda]

Thanks for the PR @faern. Do you all think we should keep this opt-out for the next release and add a note saying it will be opt-in from the release after that? I am happy to merge that way.

[faern]

Merging the PR as is is not a breaking change. If you want to go opt-in I can add a note saying that's likely what will be the case in the future.

[achanda]

Thanks all, closing this via #109