Skip to content

Conversation

@jermy-c
Copy link
Collaborator

@jermy-c jermy-c commented Dec 23, 2025

  • The NodeProcess is now a viewModel to have the same lifecycle of the app
  • Everything is now run in a IO context to coroutine to minimize blocking the UI thread
  • Files are copied in parallel now because we have multiple .js files
  • Native Methods are explicitly linked on link for performance and early error catching on load to check if the function signature matches.

@jermy-c jermy-c requested a review from benbucksch December 23, 2025 00:04
@jermy-c jermy-c self-assigned this Dec 23, 2025
Copy link
Collaborator

@benbucksch benbucksch left a comment

Choose a reason for hiding this comment

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

I cannot really review this code well, because it's too deep in Android, JNI, node.js. I looked as well as I could, but my review is necessarily only on the surface.

Looks OK to me. Just a few nits.

Only major question is: Do we need to shut down node.js?


// Register your class' native methods.
static const JNINativeMethod methods[] = {
{"startNode", "([Ljava/lang/String;)I", reinterpret_cast<jint*>(startNode)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there no shutdown needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be good to shutdown Node.js since job.cancel() does shutdown the process. Unfortunately, the node::Start() does not return an env variable required by the node::Stop() function. But I've not seen any issues so far even after restarting the app immediately.

implementation project(':capacitor-camera')
implementation project(':capacitor-filesystem')
implementation project(':capacitor-splash-screen')
implementation project(':capacitor-status-bar')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this come from?
If this is needed, it should be a separate PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was from when we added the Capacitor Status Bar plugin and the changes in this file was not committed. I'll remove this. But this will be added anyways when you do cap sync.

include ':capacitor-splash-screen'
project(':capacitor-splash-screen').projectDir = new File('../node_modules/@capacitor/splash-screen/android')

include ':capacitor-status-bar'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is also from cap sync. This is will still work even if it's not committed, do we still want to commit changes like this in the future? But I'm not sure if it'll still work if the file is not present in the git history but the changes don't need to be committed.

@jermy-c
Copy link
Collaborator Author

jermy-c commented Dec 23, 2025

I've made some fixes:

  • Fix the comments
  • Removed the change that added the capacitor-status-bar to the dependencies but still kept capacitor-nodejs removed in the build.gradle files
  • Add delete[] to delete the pointers after node::Start() exits
  • Start loadLibraries() and copyNodeAssets() in parallel but make sure both are complete before starting Node.js
  • Let FileOperations functions throw to the caller, I thought this will cause an Uncaught Exception in the coroutine but turns out it doesn't. And this is better because it stops all other coroutines if one fails, after reading about it.
  • Inlined the variables
  • Split code into functions for copyNodeAssets() and the C++ arguments function

Node.js Mobile doesn't seem to support node::Stop(). nodejs-mobile/nodejs-mobile#135

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.

3 participants