fix: detect bound require reexports in cjs analysis#328
fix: detect bound require reexports in cjs analysis#328bartlomieju merged 4 commits intodenoland:mainfrom
Conversation
|
|
| /// Analyzes the script for CommonJS exports and re-exports based on similar | ||
| /// functionality to cjs-module-lexer (https://github.com/nodejs/cjs-module-lexer). |
There was a problem hiding this comment.
Side note:
The cjs-module-lexer was replaced to merve recently.
There was a problem hiding this comment.
Good find, any chance you could compare if there are cases this new library covers that we don't cover yet?
There was a problem hiding this comment.
At first glance, it appears that line numbers are not tracked by cjs_parse.rs.
https://github.com/nodejs/merve/blob/53ee4927c0d9127110c48431882f1e3cf7ccd2ec/include/merve/parser.h#L51-L57
https://github.com/nodejs/merve/blob/53ee4927c0d9127110c48431882f1e3cf7ccd2ec/tests/real_world_tests.cpp#L1136-L1149
There was a problem hiding this comment.
Why don't we merge the implementations? I can write a C API, and we can write a rust crate for Merve. Just like Ada?
There was a problem hiding this comment.
Aligning the two implementations would be great. We have a ton of complications because we rely on swc for this.
There was a problem hiding this comment.
I'm more than happy to collaborate. I'll write C APIs for Merve. We can add Rust crate inside github.com/nodejs/merve
There was a problem hiding this comment.
That would be great! One thing is it would need to be able to be able to compile with cargo build --target=wasm32-unknown-unknown for us to use it though. We want our code to be accessible from JavaScript (ex. this does cjs export analysis: https://jsr.io/@deno/loader)
There was a problem hiding this comment.
We've tackled this same problem in Ada (in https://github.com/ada-url/rust/blob/main/build.rs). Probably we can reuse this once the time comes for adding Rust crate. I'll open a PR for C API soon. I'll reach out to you from X (x.com/yagiznizipli).
There was a problem hiding this comment.
As promised, here's the Rust crate: https://crates.io/crates/merve, and here's the pull-request: nodejs/merve#14
There was a problem hiding this comment.
Thanks! I'll try it out in the deno repo.
5 participants
Details in:
redoclyhangs and shows different results compared to Node.js output deno#31347 (comment)It seems that caused by the reexport patterns
__export(require(...))and__export(bound_ident)output by TypeScript were not detected in deno_ast layer.