feat: expose ast utilities in typescript_wasm#11973
Conversation
|
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c472f99e7c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| None => { | ||
| if input.is_instance_of::<Uint8Array>() { | ||
| let input = input.unchecked_into::<Uint8Array>(); | ||
| match input.to_string().as_string() { |
There was a problem hiding this comment.
For the newly exported parse APIs, ordinary Uint8Array input is converted with TypedArray.prototype.toString(), which joins bytes as comma-separated numbers rather than decoding UTF-8. In Node/browser callers that pass new Uint8Array(...) or TextEncoder().encode(src), parseSync will parse strings like 99,108,97... and can return a wrong AST or syntax error instead of parsing the original source; decode the byte array contents explicitly.
Useful? React with 👍 / 👎.
| .emit(); | ||
| } | ||
|
|
||
| Ok(program) |
There was a problem hiding this comment.
The existing JS-facing parse bindings resolve scopes before serializing (build_parse_sync! in crates/binding_macros/src/wasm.rs and bindings/binding_core_node/src/parse.rs both call resolver(...)), but this new API returns the raw parser result. For callers that inspect ctxt to distinguish unresolved/top-level/local identifiers or compare with @swc/core.parseSync, even simple declarations are wrong because all identifiers keep ctxt: 0; run the resolver before Ok(program).
Useful? React with 👍 / 👎.
| export declare function parse(src: string | Uint8Array, opts?: Options): Promise<Program>; | ||
| export declare function parseSync(src: string | Uint8Array, opts?: Options): Program; |
There was a problem hiding this comment.
These new parse signatures point TypeScript users at Options, but the exported Options interface from swc_ts_fast_strip does not declare the parser field that parse_program actually reads. In typed consumers that need TSX parsing, parseSync('<x />', { parser: { tsx: true } }) is rejected even though TsSyntax::tsx defaults to false and the runtime supports the option; add parser options to the public type or use a parse-specific options type.
Useful? React with 👍 / 👎.
Merging this PR will not alter performance
Comparing 
Footnotes
|
kdy1
left a comment
kdy1
left a comment
There was a problem hiding this comment.
Can we do rust-side transform/analysis code instead of js side code?
Asking because it would be really slow to pass AST from Rust to js
|
If it's really inevitable I can merge this PR, but I think it's not a good idea for performance |
|
Post nodejs/node#64034 (which removes our dependency on AST parsing for part of the REPL), we use AST parsing to:
We can probably determine if JS syntax is valid + recoverable (in the REPL) via CDP's For me, it feels weird to expose Node.js-internal-use-only Rust bindings (like |
|
I made #11975 based on the requirements of nodejs/node#64034 Note: I made it with Codex, so I need to verify it. I'm busy atm, so I can verify later this week |
|
@avivkeller Does the API in #11975 looks good to you? |
|
@avivkeller As an alternative, I can create a nodejs-only binding wasm. What do you think about it? |
|
Sorry for the delay here! Been extremely swamped, thanks for the second ping. I'd love for some Node.js-ers to give their opinions before we proceed with anything, cc @marco-ippolito @JakobJingleheimer |
|
I dont mind the experiment we can release a nightly of amaro |
|
@kdy1 both ideas look good to me, so which ever you prefer |
**Description:** Adds an independent Rust/WebAssembly binding at `bindings/binding_nodejs_support_wasm`, published as `@swc/nodejs-support-wasm`. The package has its own Rust crate, sources, build scripts, tests, README, Cargo entry, and npm workspace entry. It does not build from `binding_typescript_wasm` and does not add features or exports to the existing `@swc/wasm-typescript` or `@swc/wasm-typescript-esm` packages. The new CommonJS package embeds its wasm binary and exposes these APIs at the top level: - `transform` - `transformSync` - `transformModuleSyntax` - `getFirstExpression` - `isValidSyntax` - `isRecoverableError` It also exports the related `Options`, `TransformOutput`, and `ModuleSyntaxTransformOutput` TypeScript types. The npm publish matrix, wasm CI matrix, release versioning, Cargo lockfile, and pnpm lockfile now reference the standalone `binding_nodejs_support_wasm` crate. Validation performed: - `git submodule update --init --recursive` - `cargo fmt --all -- --check` - `cargo test -p binding_typescript_wasm -p binding_nodejs_support_wasm` - `cargo clippy --all --all-targets -- -D warnings` - `(cd bindings/binding_nodejs_support_wasm && ./scripts/test.sh)` - `(cd bindings/binding_typescript_wasm && ./scripts/test.sh)` - `(cd bindings/binding_core_wasm && ./scripts/test.sh)` - `(cd bindings/binding_minifier_wasm && ./scripts/test.sh)` - `(cd bindings/binding_es_ast_viewer && ./scripts/test.sh)` - `(cd bindings/binding_nodejs_support_wasm/pkg && npm pack --dry-run --json)` - `pnpm install --frozen-lockfile --ignore-scripts` - `go run github.com/rhysd/actionlint/cmd/actionlint@latest .github/workflows/CI.yml .github/workflows/publish-npm-package.yml` **BREAKING CHANGE:** None. The existing TypeScript wasm packages and their public APIs are unchanged. **Related issue (if exists):** - Supports nodejs/node#64034 - Follow-up context from #11973
4 participants
Currently, https://github.com/nodejs/node relies on two separate AST parsers, as SWC does not expose it's AST bindings to JS-land. This PR changes SWC to expose those bindings, allowing for consumers, like node core, to parse AST using SWC.
See: nodejs/amaro#435