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

feat(neon-macros): Add neon::main proc attr macro #636

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Nov 19, 2020

Summary

Adds a #[neon::main] proc macro.

tl;dr

Use an attribute to initialize either a legacy or napi neon module. The fn doesn't even need to be pub!

use neon::prelude::*;

#[neon::main]
fn main(mut cx: ModuleContext) -> NeonResult<()> {
    Ok(())
}

Migration

diff --git a/test/dynamic/native/src/lib.rs b/test/dynamic/native/src/lib.rs
index 3f2c937..409ab55 100644
--- a/test/dynamic/native/src/lib.rs
+++ b/test/dynamic/native/src/lib.rs
@@ -20,7 +20,8 @@ use js::classes::*;
 use js::tasks::*;
 use js::eventhandler::*;

-register_module!(mut cx, {
+#[neon::main]
+fn main(mut cx: ModuleContext) -> NeonResult<()> {
     cx.export_function("return_js_string", return_js_string)?;

     cx.export_function("return_js_number", return_js_number)?;
@@ -84,4 +85,4 @@ register_module!(mut cx, {
     cx.export_class::<JsPanickyConstructor>("PanickyConstructor")?;

     Ok(())
-});
+}

Thought Process

neon-macros

Procedural macros must be in their own crate. A new crate, neon-macros, is created. The crate has a single feature flag, napi for toggling between legacy and napi implementations.

Proc macros must be at the root of a crate. To keep the structure clean, actual implementations are in napi.rs and legacy.rs modules that are conditionally imported to lib.rs. The lib.rs has thin glue code for delegating to the correct implementation.

Re-export

Proc macros are re-exported from neon to keep users from needing to specify another crate.

Feature Flags

Proc macros can have a negative impact on compile time. A new feature flag, proc-macros is added to enable the feature. It is default disabled since most legacy users will continue to use the normal macros.

It is planned for proc macros to be the recommended way to use N-API. Therefore, the napi-runtime feature flag automatically enables the proc-macros feature flag.

Open Questions

  1. Should we keep the panic hook? I think it should be removed. In my opinion, it's more harmful than helpful; especially since it is global. The hook is a holdover from earlier implementations and is no longer necessary. It will be removed.
  2. Naming. I like neon::init because it initializes and can be called multiple times (once per context). However, neon::main is another option. Consensus on neon::main.
  3. Should we try to protect against using #[neon::init] multiple times? I think we shouldn't because it would be complicated and brittle for limited value. This can be a follow-up.

) -> ::neon::macro_internal::runtime::nodejs_sys::napi_value {
// Suppress the default Rust panic hook, which prints diagnostics to stderr.
#[cfg(not(feature = "default-panic-hook"))]
::std::panic::set_hook(::std::boxed::Box::new(|_| { }));

This comment was marked as resolved.

@kjvalencik kjvalencik force-pushed the kv/proc-macro branch 2 times, most recently from 4cc9651 to 60d4a7d Compare November 19, 2020 23:17
@kjvalencik

This comment has been minimized.

@kjvalencik kjvalencik force-pushed the kv/proc-macro branch 2 times, most recently from cdeede9 to ef00781 Compare November 23, 2020 15:41
@kjvalencik
Copy link
Member Author

Updated with changes from feedback.

@kjvalencik kjvalencik changed the title feat(neon-macros): Add neon::init proc attr macro feat(neon-macros): Add neon::main proc attr macro Nov 23, 2020
Copy link
Member

@goto-bus-stop goto-bus-stop 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 it

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