DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

Change `embed` macro. #144

Closed DelSkayn closed 1 year ago

DelSkayn commented 1 year ago

The current embed macro semantics are a bit strange.

The macro is used as following:

#[embed(path = "./path/to/files")]
mod module_name{}

This creates a bundle loaded into as static with name BUNDLE_NAME and try to load module_name.js from the ./path/to/files relative to the root of the crate. Multiple files can be loaded by using the name attribute. The resulting bundles is both a loader and a resolver.

This behavior is somewhat strange as we use the mod keyword to declare the bundle but a bundle isn't a module. The current implementation is also a bit of a hack as we actually use quickjs to resolve the files and hijack the resolver and loader to retrieve the loaded modules.

I think a better method of implementing this macro something like the following:

const BUNDLE: Bundle = embed!{
    "./path/to/files/module_name.js": "module_name",
}

This makes it more clear that it creates a bundle and where it is loaded from.

DelSkayn commented 1 year ago

Closing as it was implemented in #146

katyo commented 1 year ago

@DelSkayn

I think the colon between path and module name may confuse. I propose use => instead or swap module name and path.

const BUNDLE: Bundle = embed!{
    "./path/to/files/module_name.js" => "module_name",
    "./path/to/files/module_name.js",
}
// or
const BUNDLE: Bundle = embed!{
    "module_name": "./path/to/files/module_name.js",
    "./path/to/files/module_name.js",
}
DelSkayn commented 1 year ago

I initially thought to go with => but I thought I would take inspiration from how rust binding works.

Like for example:

let Some::Binding{ place:  rename } = bar;

In this binding the value in place is declared as rename. I found this to be similar to what the macro is doing.

What do you think would make using : confusing?

katyo commented 1 year ago

@DelSkayn It confused for me because this is not looks like a case of binding. Looks like an assignment the value to the field. So I propose swap module name and path.

DelSkayn commented 1 year ago

@katyo alright, fair enough, I will swap the name and path.