search

smol-rs / async-broadcast

Async broadcast channels
Apache License 2.0
151 stars 26 forks source link

(breaking change) Add std and alloc features #65

Open notgull opened 4 weeks ago

notgull commented 4 weeks ago

These features do nothing and raise a compiler error if disabled. However this enables us to disable these features in the future.

zeenix commented 4 weeks ago

Firstly, why is this a breaking change? :thinking:

These features do nothing and raise a compiler error if disabled. However this enables us to disable these features in the future.

While I understand the motivation here, having std listed as a supported feature will give folks the wrong idea and there is nothing worse than empty promises. This is made worse by the fact that list of features in docs.rs doesn't have a description. We can certainly document this well but AFAIK people often look at the feature section on docs.rs. We also need to be prepared for the possibility that nostd is never added to this repo for any reason.

There is no hurry on https://github.com/smol-rs/smol/issues/31 so if you are confident that you can add the implementation soonish, we can hold off on 1.0 for now.

notgull commented 4 weeks ago

Firstly, why is this a breaking change? 🤔

If a crate has this in its Cargo.toml it will now fail to compile.

[dependencies]
async-broadcast = { version = "0.7", default-features = false }

They will need to change it to:

[dependencies]
async-broadcast = { version = "0.7", default-features = false, features = ["std"] }

While I understand the motivation here, having std listed as a supported feature will give folks the wrong idea and there is nothing worse than empty promises. This is made worse by the fact that list of features in docs.rs doesn't have a description.

I don't think this is an issue in practice: crossbeam-channel does this as well and I don't think users file issues about it.

We also need to be prepared for the possibility that nostd is never added to this repo for any reason.

As a counterpoint, we need to prepare for the possibility that it will.

This is largely about stability. If we release v1.0 now and then add a no-std impl later, we'll need to make another breaking change to make it no-std compatible.

zeenix commented 3 weeks ago

If a crate has this in its Cargo.toml it will now fail to compile.

Oh I missed the compiler error you inserted. Given we don't have any features currently, I think this is only breaking in theory.

I don't think this is an issue in practice

I disagree with the argument (once you realise that the feature is dummy, there is no reason to file an issue) but I think the compiler error you added, will prevent people from making a mistake. It's just very annoying to find out that way though.

As a counterpoint, we need to prepare for the possibility that it will.

Sure but since we don't have any features (let alone default ones), I don't think this will be a breaking change in practice.

notgull commented 3 weeks ago

Given we don't have any features currently, I think this is only breaking in theory.

No, this is breaking in practice. If anyone does import async-broadcast with no-default-features it will break their build.

zeenix commented 3 weeks ago

No, this is breaking in practice. If anyone does import async-broadcast with no-default-features it will break their build.

Yes but they won't and shouldn't because there are no features to disable. If people do this for no reason, of course their builds will break later. Should we really confuse others with a dummy feature for people who do that? I don't think so.

zeenix commented 3 weeks ago

I don't think so.

But if you strongly disagree and want to cater for people who disable default features out of habit or by mistake, I think it's easier to just wait till we have nostd implemented or we can decide not to go nostd in the near future. Those are the only choices IMO.

Btw I learnt about embassy_sync::pubsub::PubSubChannel yesterday. Might be good to check that out.