-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: add support for ECMAScript modules in the JavaScript kit #236
Conversation
5778e85
to
fe7ba0e
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.
LGTM as a first approach, I think we can improve later.
kits/javascript/src/main.rs
Outdated
@@ -49,7 +49,19 @@ fn main() { | |||
}, | |||
} | |||
|
|||
context.eval_global("handler.js", &contents).unwrap(); | |||
// Checks if the given code uses ECMAScript modules. | |||
if contents.contains("export default") { |
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 guess this is fine for our purposes now, but this would identify a comment "// export default" as so, even if it wasn't really JS code.
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 are totally right. This is a limitation on the current implementation. For now, I believe it's fine as the biggest issue should be a failed response from wws
. Detecting a comment would be more complex as you can write multi-line comments in JS with the /* ... */
syntax. We would need to detect the following code too:
/*
export default app;
*/
This is a task for a JS parser, but I would avoid adding this dependency unless is strictly necessary. The reason is that it adds complexity and affects the performance. If we find more edge cases that requires it, we may decide to add the parser like the Javy CLI tool.
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 also added a third regex to detect exported objects like:
export default {
async fetch() {
// ...
}
}
LGTM modulo the windows CI |
Thanks to the new Javy version, we can now eval JavaScript modules with the
eval_module
method. To use this new method, I had to update thejavy
version to 2.0.0. The upgrade caused some errors in the build process due to crate incompatible versions (wasmtime-wasi
requiresbytes = "^1.4"
whilejavy
requiresbytes = "=1.1.0"
. Since I consider these two projects as separate entities, I just excluded it from the workspace.Finally, I added a new
js-modules
example that showcases it.It closes #149