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

Make build() fn's closure return a Result #1064

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

Conversation

akonradi-signal
Copy link
Contributor

This is a pure refactor that makes the build function more idiomatic; no externally-facing types or functions are changed. The big benefit here is that it makes it possible to, in a future change, make the return type of some functions infallible, like JsError::error.

@@ -59,18 +59,13 @@ pub use self::promise::JsFuture;

// This should be considered deprecated and will be removed:
Copy link
Member

Choose a reason for hiding this comment

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

Note this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was noted; I hadn't seen a plan for removal and went for a safe refactor that enabled what I really wanted: infallible JsError::error. Is that end goal even possible and, if so, is it worth blocking on the larger cleanup for?

@kjvalencik
Copy link
Member

kjvalencik commented Sep 9, 2024

While I appreciate the refactor--cleaning up is always good for maintenance!--the build function is an artifact of when Neon supported two different backends: one that used NAN/C++ and one that used Node-API.

Since it shouldn't be used in anything new and should eventually be completely removed, I would prefer not to change it.

This is likely where I would start in that refactor.

Thanks!

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