-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
WIP V3: Refactor RPC #9802
WIP V3: Refactor RPC #9802
Conversation
44578e4
to
7693937
Compare
302359e
to
02b3eb7
Compare
Tidy tidy
RpcFileSystem
02b3eb7
to
1b5ddc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event names can be shared with JavaScript using:
#[napi]
pub enum FileSystemEvent {
ReadToString = 1,
IsFile,
IsDir,
}
impl FileSystem for RpcFileSystemNodejs {
fn read_to_string(&self, path: &Path) -> io::Result<String> {
self.rpc_host.send::<PathBuf, String>(FileSystemEvent.ReadToString, path.to_path_buf())
}
}
import { FileSystemEvent } from '@parcel/rust'
console.log(FileSystemEvent.IsDir) // 3
Though it might get a bit difficult with clashing events (given this assigns them as numbers). Otherwise we can use strings and add a prefix
type IsFileHandler = HandlerFunc<'fs/is_file', string, boolean>; | ||
type IsDirHandler = HandlerFunc<'fs/is_dir', string, boolean>; | ||
|
||
export class FileSystemRpc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like a function would be a cleaner choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so too, though I am wondering if we need to have methods on it - perhaps a .destroy()
to detach listeners
@@ -23,7 +22,7 @@ pub struct ParcelNapiBuildResult {} | |||
pub struct ParcelNapiOptions { | |||
pub threads: Option<u32>, | |||
pub node_workers: Option<u32>, | |||
pub fs: Option<JsObject>, | |||
pub use_file_system: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_rpc_file_system
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the FileSystem object isn't being passed into Rust from JavaScript. The ParcelV3 public API accepts a FileSystem object which gets mapped in JS to events from the RPC system.
If an FS is supplied to the public options, then we know we are using RPC to delegate calls to the FileSystem so we need to tell the rust-side ParcelNapi to send FS events to JS via RPC.
this.#routes.set(event, callback); | ||
} | ||
|
||
callback: any = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the any
here needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flow wanted this to property to be typed and for some reason it couldn't infer the type from the assignment 🤷
err: any, | ||
id: string, | ||
data: any, | ||
done: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done can be typed a little bit more
@@ -17,11 +17,11 @@ use super::RpcWorkerNodejs; | |||
// RpcHostNodejs has a connection to the main Nodejs thread | |||
pub struct RpcHostNodejs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As inverted this ;we'll always suffix trait implementations with the trait name.
This should be NodejsRpcHost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I still think it's better to use delegation rather than events like this. Current multi-threaded function FS implementation has no boilerplate and should be equivalent for our use.
Ultimately I don't think it'll be too different functionality wise, but it'll be easier to write and has a few benefits as there is no boilerplate.
Some potential use-cases like WASM may not immediately work, but I'd not optimise for those & they can be updated later on.
} | ||
}; | ||
|
||
#on_event(id: string, data: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can type everything on the file
also could we use camel case for all JS symbols?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't even realize! Haha going between Rust and JavaScript 😅 Maybe we should have a lint rule for JavaScript variable declarations
I don't understand why channels would be required on WASM because WASM can't be multi-threaded anyhow. |
Can we just call different functions directly from rust instead of emitting events? I liked your |
This PR is a bit of an RFC to help visualize & discuss what an events pattern would look like - and I am starting to feel like the more direct approach will be more ergonomic. The original intention was to have an abstraction for bindings (be it napi, wasm or a whatever) that makes it easy to support external execution environments without needing anything specific within core to change - e.g. an But after applying it, it doesn't seem like there is much merit to this. At least for things like |
You can use a single threaded async runtime on wasm so you can use async channels without blocking the main thread. There are also approaches to use a web Worker farm with Rayon or Tokio to get true multi-threading working - though the host needs to have some restrictive security headers in order to use threads. I assume our usage of wasm won't be for serious applications, only for the playground, so it doesn't really matter that much. |
Closing this in favour of calling JS a little more directly from napi |
This more of an RFC - I'm not sure I'm convinced by the RPC implementation (even though I wrote it initially lol)
In this PR I refactor the RPC implementation to reduce the boilerplate and make it more versatile.
I removed the
RpcMessage
enum and instead have theRpcHost
expose a publicsend
method that usesserde
to send/receive types.On the Rust side:
On the JavaScript side
This should make it easier to create external users of the Rpc implementation (like FS and Cache)
Changelog
RpcConnection
toRpcWorker
RpcWorker
, calling theThreadSafeFunction
directlyRpcHost
andRpcWorker
RpcHost.send()
andRpcWorker.send()
method that usesserde
to convert types in/out of JavaScriptsend_serde
will automatically typecastsend_with
will let you manually map the parameters and return valueRpcHostMessage
&RpcConnectionMessage
enums