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

Expose async-attributes macros behind feature flag #237

Closed
yoshuawuyts opened this issue Sep 24, 2019 · 10 comments · Fixed by #238
Closed

Expose async-attributes macros behind feature flag #237

yoshuawuyts opened this issue Sep 24, 2019 · 10 comments · Fixed by #238
Labels
api design Open design questions

Comments

@yoshuawuyts
Copy link
Contributor

For applications it's generally useful to have access to the attributes as defined in async-attributes. Being able to install a single dependency seems quite desirable.

Implementation

I propose we expose these attributes from the root of the crate, relying on a feature flag. As for the flag name, there's a few options. But the one I think I prefer is "bin" to indicate this should be used for binaries only.

#[cfg(feature = "bin")]
pub use async_attributes::*;
@yoshuawuyts yoshuawuyts added the api design Open design questions label Sep 24, 2019
@yoshuawuyts
Copy link
Contributor Author

Oh wait actually they do. The bin border just doesn't show up. Looks like the re-export bug got fixed also!

Screenshot_2019-09-24 async_std - Rust(1)

@yoshuawuyts
Copy link
Contributor Author

Okay so this is terrible, but thanks to async-rs/async-attributes#8 we can properly render the bin attribute on docs.

Screenshot_2019-09-24 async_std - Rust(3)

@yoshuawuyts
Copy link
Contributor Author

Oops, I meant to post all these screenshots on the PR #238. Sorry sorry.

@ghost
Copy link

ghost commented Sep 24, 2019

Should this also be behind the unstable feature flag? Or is the bin flag itself suppose to be unstable?

If the motivation for another feature flag is reducing compilation times, I wonder if we should just embrace sync and quote dependencies to simplify things...

@yoshuawuyts
Copy link
Contributor Author

@stjepang For me "unstable" means: "we want to eventually find a way to stabilize this, or remove it if we don't think it's useful. To me "bin" means: this is behind a feature flag forever because we're polyfilling the language.

I wonder if we should just embrace sync and quote dependencies to simplify things...

It would certainly make things easier; in particular #225 could greatly be simplified. And our ideas around random delays / tracing would be furthered by it. So not sure; we could consider it? But that feels like a larger discussion than what we're talking about here. And even then having this live behind a flag may still be desirable.

@yoshuawuyts
Copy link
Contributor Author

And even then having this live behind a flag may still be desirable.

Actually thinking more about it; I don't think people care about splitting lang polyfills out into separate features. If anything feature flags are kind of annoying. I think you're right, and this flag should only exist to guard pulling in the syn + quote dependencies.

@skade
Copy link
Collaborator

skade commented Sep 29, 2019

I'm not convinced of exposing these features through the async-std library. I don't quite see the argument for just deploying one single library with multiple features. While it's desirable to ship e.g. an official test macro, it also leads to the jarring situation where alternative test frameworks need to deactivate such a feature.

@yoshuawuyts
Copy link
Contributor Author

it also leads to the jarring situation where alternative test frameworks need to deactivate such a feature.

Can you share more about this? This is the first time I'm hearing of it.

@ghost
Copy link

ghost commented Nov 2, 2019

@skade Could you expand on the problem? It'd be nice to have this behind the attributes feature flag (or something similar).

@ghost ghost closed this as completed in #238 Nov 7, 2019
@thomaseizinger
Copy link

The docs of async-attributes currently say:

We want to make sure async-std's surface area is stable, and only includes things that would make sense to be part of "an async version of std". Language level support is really important, but not part of the standard library.

This has some distinct benefits: in particular it allows us to version both crates at a different pace. And as features are added to the language (or we decide they weren't a great idea after all), we can incrementally shrink the surface area of this crate.

The other big benefit is that it allows libraries to depend on async-std without needing to pull in the rather heavy syn, quote, and proc-macro2 crates. This should help keep compilation times snappy for everyone.

I understand the second point but the first one is no longer true with the re-export, is it? Even with the feature-flag, the attributes are now part of the public API of async-std and hence it would be a breaking change to remove them in favor of language features, right?

It feels like we could just merge those macros into async-std now to remove this indirection (and also remove the circular dependency: async-attributes has a devDependency on async-std).

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api design Open design questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants