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

Rename EventQueue to Channel #734

Closed
wants to merge 4 commits into from
Closed

Rename EventQueue to Channel #734

wants to merge 4 commits into from

Conversation

dherman
Copy link
Collaborator

@dherman dherman commented May 15, 2021

This PR experiments with renaming the event queue API:

  • neon::event::EventQueue -> neon::event::Channel
  • neon::event::EventQueueError -> neon::event::SendError

This is based on an insight @kjvalencik made recently: a clone/copy/sync-able data structure that gives access across multiple threads to the Node.js runtime's internal event queue by a send operation that communicates across threads is much closer conceptually to Rust's idiomatic concept of channels than it is to being an abstraction of the JS event queue itself.

The PR attempts to make the change conservatively, preserving the previous names as aliases for the new names but adding deprecation attributes and hiding them from API docs. This allows us to ship the new name without breaking existing customers and eventually remove the deprecated names in a future release.

Here's a preview of updated neon::event docs with code examples where you can see how the code looks with the renamed API.

dherman added 3 commits May 15, 2021 12:45
The previous types are preserved for backwards compatibility.
…compatibility.

I think this will result in fewer deprecation warnings for users than ideal, but it seems better to optimize for compatibility than warnings.
@dherman dherman force-pushed the rename-event-queue branch from 10e980b to 6221ee2 Compare May 15, 2021 20:22
@dherman dherman marked this pull request as ready for review May 15, 2021 20:56
@dherman dherman requested a review from kjvalencik May 15, 2021 20:56
src/prelude.rs Show resolved Hide resolved
Copy link
Member

@kjvalencik kjvalencik left a comment

Choose a reason for hiding this comment

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

I really, really like this naming change. I think it clarifies a lot. It also resolves some naming questions for when we implement bounded queues. We can call it cx.sync_channel().

@dherman
Copy link
Collaborator Author

dherman commented May 18, 2021

I really, really like this naming change. I think it clarifies a lot.

Right?? I'm so glad you brought this idea up.

It also resolves some naming questions for when we implement bounded queues. We can call it cx.sync_channel().

Oh, I like that. I know it's FCP, but we should probably update neon-bindings/rfcs#32 with the new name and a note about sync_channel() listed as a possible future extension.

@dherman
Copy link
Collaborator Author

dherman commented May 22, 2021

@kjvalencik Do you think this is ready to merge?

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