-
Notifications
You must be signed in to change notification settings - Fork 285
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
Replace all generic contexts with a single Cx type #1048
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. I left a suggestion for a little documentation in the neon::context
module docs.
The only thing I don't like is the collective baggage of all the different Context types. I wonder if it's worth making some of them doc(hidden)
? Or if there's some way to structure the neon::context
docs to make it easier to understand the minimal subset you actually need to learn?
f2fad0c
to
bdf0183
Compare
@dherman I marked them |
889b75b
to
d14c1e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, let's ship it!
This allows users to take
Cx
as an argument instead of being generic over theContext
trait. For example, this will allow us to implementTryIntoJs
for closures.This is technically a breaking change because several context types have been replaced by type aliases; if a user were implementing a trait over multiple of these types, it would now cause a conflict. In practice, users either didn't do this at all or implemented it generically over the
Context
trait.We could keep the existing types, it just makes it harder to deprecate them in the future. For a Neon 2.0, I would want to eliminate the
Context
trait and only make it concrete. The special needs of function and module would become additional arguments.