Skip to content

RFC: add pyo3-like attribute macros to define nodejs classes and methods#43

Open
Mossaka wants to merge 2 commits into
neon-bindings:mainfrom
Mossaka:main
Open

RFC: add pyo3-like attribute macros to define nodejs classes and methods#43
Mossaka wants to merge 2 commits into
neon-bindings:mainfrom
Mossaka:main

Conversation

@Mossaka

@Mossaka Mossaka commented Jun 3, 2021

Copy link
Copy Markdown

Signed-off-by: Joe Zhou jiazho@microsoft.com

Signed-off-by: Joe Zhou <jiazho@microsoft.com>
@Mossaka Mossaka changed the title RFC to add pyo3-like attribute macros to define nodejs classes and methods RFC: add pyo3-like attribute macros to define nodejs classes and methods Jun 3, 2021

@kjvalencik kjvalencik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks so much for this proposal! Py03 is a great example because a lot of the problem space are shared.

name: string,
age: u8,
}
#[neon::methods]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think these should be namespaced to help prevent conflicts with other macros. This could be named neon::class_methods or we could have everything in a neon::class module.

#[neon::class::data]
#[neon::class::methods]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Currently neon only has one proc/attribute macro, right? main

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's correct.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I actually prefer the simplicity of #[neon::methods], #[neon::classes] if in a foreseeable future there won't be any namespace conflicts.

Ok(cx.string(format!("Hello, my name is {}", self.name)))
}

#[neon::constructor]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current declare_types macro has a two-part initialization with the second part being optional.

  1. (Required). Constructor. This creates the Rust struct that gets wrapped into the JS class.
  2. (Optional). Initialization. This has access to the created class and can perform actions like assigning properties. This can be thought of as using this in a JS constructor.

I think this is valuable and it would be nice to maintain that functionality. I can think of a couple ways to implement it:

  1. Another macro attribute like #[neon::class_init] for that phase
  2. Need to call a function and return a class instead of the data type.
create_class(Person { name, age })

- What other designs have been considered and what is the rationale for not choosing them?
- What is the impact of not doing this?

# Unresolved questions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some unresolved questions that I have.

  1. What is the name of the type? Do we generate complete new types or do we have something like JsClass<Person>?
  2. Do we implicitly create a RefCell or do we only allow &self? I think we should start simple and class methods can only take &self. If they take a different form of &self we fail to compile and if they don't take self at all we create a static method.
  3. Are there any other critical features? In my opinion we should start simple and not try to add everything.
  4. What happens if the class is called as a function instead of a constructor?

Signed-off-by: Joe Zhou <jiazho@microsoft.com>
@Mossaka

Mossaka commented Jun 9, 2021

Copy link
Copy Markdown
Author

Updated RFC with the comments.

@esatterwhite

Copy link
Copy Markdown

Would love to see this added back. Very much missing it from the recent versions

@louneskmt

Copy link
Copy Markdown

Any update on this RFC?

@kjvalencik

Copy link
Copy Markdown
Member

@louneskmt No update. This RFC itself is blocked by being able to create classes (i.e., a type safe wrappers around napi_define_class and napi_wrap).

While this feature is highly desirable, it's not considered a blocker for Neon 1.0 because the functionality can be accomplished with some JavaScript glue code. Blockers for 1.0 have taken priority.

Contributions would be welcome. I have spent a bit of time thinking about what Neon's napi_define_class might look like. Most likely:

  • A trait providing a constructor, name, and properties
  • An instance data cache that can lookup constructors based on the implementing type
  • Some code for wrap/unwrap that operates similar to JsBox

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.

4 participants