[TS] Export reducers, etc from a module #4220
Conversation
coolreader18
requested review from
bfops,
cloutiertyler and
jdetter
as code owners
coolreader18
force-pushed
the
noa/ts-exports
branch
3 times, most recently
from
115f08a to
ac98ef9
Compare
|
|
||
| let maj_sys_ver = maj_sys_ver.context( | ||
| "Your module doesn't import the `spacetimedb/server` package at all - \ | ||
| this is likely a mistake, as your module will not be able to do anything", |
There was a problem hiding this comment.
| this is likely a mistake, as your module will not be able to do anything", | |
| this is likely a mistake, as your module will not be able to interface with the SpacetimeDB host.", |
| if maj_sys_ver == 2 { | ||
| anyhow::ensure!( | ||
| output_chunk.exports.contains(&"default".into()), | ||
| "It seems like you haven't exported your schema. You must `export default spacetime;`" |
There was a problem hiding this comment.
| "It seems like you haven't exported your schema. You must `export default spacetime;`" | |
| "It seems like you haven't exported your schema. You must `export default schema(...);`" |
Centril
left a comment
Centril
left a comment
There was a problem hiding this comment.
Some initial thoughts. I'd also appreciate a longer PR description for this one :)
| /// The actual callable module hook functions and their abi version. | ||
| pub(in super::super) struct HookFunctions<'scope> { | ||
| pub abi: AbiVersion, | ||
| pub recv: Local<'scope, v8::Value>, |
There was a problem hiding this comment.
This field is non-obvious, would be good to add a doc comment.
| let eval_steps = |context, module| { | ||
| ($scope:expr, $module_name:expr $(, ($($fun:tt)*))* $(,)?) => {{ | ||
| let export_names = &[$(create_synthetic_module!(@export_name ($($fun)*)).string($scope)),*]; | ||
| let eval_steps = |context: v8::Local<v8::Context>, module: v8::Local<v8::Module>| { |
There was a problem hiding this comment.
Local is already imported. For v8:: stuff, our code does not qualify stuff with v8::, so let's keep these as unqualified paths. (Applies generally in the PR.)
| (@export_name ($name:ident = $value:expr)) => { | ||
| str_from_ident!($name) | ||
| }; | ||
| (@register $scope:expr, $module:expr, ($name:ident = $value:expr)) => { |
There was a problem hiding this comment.
For me, the @private_thing convention for macros produces code that's hard to read as it requires you to first understand the macros syntax and should imo only be used when it's needed for exporting macros from crates and then it should be well documented. Here, that's unnecessary, as it's all internal to the module. Instead, more macros can be made.
| default_export: Local<'_, v8::Value>, | ||
| exports_obj: Local<'_, v8::Object>, |
There was a problem hiding this comment.
| default_export: Local<'_, v8::Value>, | |
| exports_obj: Local<'_, v8::Object>, | |
| default_export: Local<'_, Value>, | |
| exports_obj: Local<'_, Object>, |
| // Convert `hooks` to an object. | ||
| let hooks = cast!(scope, args.get(0), Object, "hooks object").map_err(|e| e.throw(scope))?; | ||
| let hooks_fn = default_export | ||
| .try_cast::<v8::Object>() |
There was a problem hiding this comment.
| .try_cast::<v8::Object>() | |
| .try_cast::<Object>() |
|
|
||
| // Find the `__call_reducer__` function. | ||
| let hook_functions = get_hooks(scope).context("The `spacetimedb/server` module was never imported")?; | ||
| // Find the `__call_reducer__` function. |
There was a problem hiding this comment.
Comment needs an update, not just call_reducer anymore.
| Either::Right(mcc) => (&mcc.replica_ctx, &mcc.scheduler), | ||
| }; | ||
| let instance_env = InstanceEnv::new(replica_ctx.clone(), scheduler.clone()); | ||
| scope.set_slot(JsInstanceEnv::new(instance_env)); |
There was a problem hiding this comment.
If I'm not mistaken, the change in ordering also changes semantics such that e.g., __describe_module__ now is able to make syscalls. This does match WASM, so this OK.
| } | ||
| }; | ||
|
|
||
| // Setup the instance common and environment. |
| // Setup the instance common and environment. | ||
| let info = &module_common.info(); | ||
| let mut instance_common = InstanceCommon::new(&module_common); | ||
| let replica_ctx: &Arc<ReplicaContext> = module_common.replica_ctx(); |
There was a problem hiding this comment.
Already in scope now? This is unnecessary now?
| scope: &mut PinScope<'scope, '_>, | ||
| body: impl FnOnce(&mut PinScope<'scope, '_>) -> Result<T, ErrorOrException<ExceptionThrown>>, | ||
| ) -> Result<T, ErrorOrException<JsError>> { | ||
| tc_scope!(scope, scope); |
There was a problem hiding this comment.
We're making two tc_scopes here now... that doesn't seem right?
There was a problem hiding this comment.
Whoops, yep, that was a mistake.
coolreader18
force-pushed
the
noa/ts-exports
branch
from
ac98ef9 to
8eaf754
Compare
3 participants
Description of Changes
Haven't changed
schema()to accept an object yet, maybe that's for a followup?Expected complexity level and risk
Testing