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

Replace all generic contexts with a single Cx type #1048

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented May 30, 2024

This allows users to take Cx as an argument instead of being generic over the Context trait. For example, this will allow us to implement TryIntoJs 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.

@kjvalencik kjvalencik changed the title Replace all generic contexts with a single Ctx type Replace all generic contexts with a single Cx type Jun 3, 2024
Copy link
Collaborator

@dherman dherman left a 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add an example of writing helper functions on contexts in the module docs, right before the Memory Management section.

}

impl<'a> UnwindSafe for ModuleContext<'a> {}
impl<'cx> DerefMut for ModuleContext<'cx> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean if you already have an &mut ModuleContext you'd have to do something like &mut *cx to pass it to a function that takes Cx?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants