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

Merged
merged 7 commits into from
Aug 28, 2024
Merged

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?

crates/neon/src/context/mod.rs Show resolved Hide resolved
crates/neon/src/context/mod.rs Show resolved Hide resolved
@kjvalencik
Copy link
Member Author

@dherman I marked them #[doc(hidden)] and removed all references to them internally. In the future, we will likely want to mark them deprecated.

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.

Looks great, let's ship it!

@kjvalencik kjvalencik merged commit dae95b1 into main Aug 28, 2024
9 checks passed
@kjvalencik kjvalencik deleted the kv/generic-ctx branch August 28, 2024 12:29
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.

2 participants