Conversation
Uses js_sys::Object to build the result directly instead of double-serializing through serde_json::Value. Statements and comments are both serialized via serde_wasm_bindgen separately.
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support for extracting source comments (with spans/locations) alongside the parsed SQL AST, exposing the functionality through the TS API and WASM bindings.
Changes:
- Expose a new
parseWithCommentsAPI (instance, static, and top-level) that returns{ statements, comments }. - Add TypeScript types for returned comments/spans and export them from the public types barrel.
- Update the Rust WASM layer to parse comments and to tokenize with location info; add
js-systo construct the JS return object.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wasm.ts | Extends the WasmModule interface with parse_sql_with_comments. |
| src/types/index.ts | Re-exports the new comments types. |
| src/types/comments.ts | Adds SourceComment and ParseWithCommentsResult public types. |
| src/parser.ts | Adds parseWithComments APIs (instance/static/top-level) calling the new WASM export. |
| src/lib.rs | Adds parse_sql_with_comments WASM export; switches tokenization to include locations. |
| src/index.ts | Re-exports parseWithComments from the package entrypoint. |
| Cargo.toml | Adds js-sys dependency for JS object construction in WASM. |
| Cargo.lock | Locks js-sys addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { dialectFromString, GenericDialect } from './dialects.js'; | ||
| import { ParserError } from './types/errors.js'; | ||
| import type { Statement } from './types/ast.js'; | ||
| import type { SourceComment, ParseWithCommentsResult } from './types/comments.js'; |
There was a problem hiding this comment.
SourceComment is imported but never used. With the repo’s unused-imports/no-unused-imports rule enabled, this will fail linting. Remove SourceComment from the import or use it in a type position in this file.
| import type { SourceComment, ParseWithCommentsResult } from './types/comments.js'; | |
| import type { ParseWithCommentsResult } from './types/comments.js'; |
| /** Parse SQL and return both AST and source comments */ | ||
| parseWithComments(sql: string): ParseWithCommentsResult<Statement> { | ||
| const wasm = getWasmModule(); | ||
| try { | ||
| return wasm.parse_sql_with_comments(this.dialect.name, sql) as ParseWithCommentsResult<Statement>; | ||
| } catch (error) { | ||
| throw ParserError.fromWasmError(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
parseWithComments() ignores this.options (e.g., recursionLimit/trailingCommas) even when the caller configured them via withOptions() / withRecursionLimit(), unlike parse(). This is a behavioral inconsistency that can surprise users. Either plumb options through to the WASM API (e.g., add a parse_sql_with_comments_with_options entrypoint) or make this a static-only helper / explicitly reject or document non-empty options.
| /** | ||
| * Parse SQL and return both AST and source comments | ||
| */ | ||
| export function parseWithComments(sql: string, dialect: DialectInput = 'generic'): ParseWithCommentsResult<Statement> { |
There was a problem hiding this comment.
New public API (parseWithComments) is introduced without corresponding tests. There is existing Vitest coverage for parse/format/validate; add tests that exercise comment extraction and span fields to prevent regressions (including build-export checks if applicable).
| export function parseWithComments(sql: string, dialect: DialectInput = 'generic'): ParseWithCommentsResult<Statement> { | |
| function parseWithComments(sql: string, dialect: DialectInput = 'generic'): ParseWithCommentsResult<Statement> { |
| js_sys::Reflect::set(&obj, &"statements".into(), &stmts_val).unwrap(); | ||
| js_sys::Reflect::set(&obj, &"comments".into(), &comments_val).unwrap(); |
There was a problem hiding this comment.
js_sys::Reflect::set(...).unwrap() can panic in WASM, aborting the module on what should be a recoverable error path. Prefer propagating the JsValue error (e.g., map Reflect::set errors into the Result) instead of unwrapping.
| js_sys::Reflect::set(&obj, &"statements".into(), &stmts_val).unwrap(); | |
| js_sys::Reflect::set(&obj, &"comments".into(), &comments_val).unwrap(); | |
| js_sys::Reflect::set(&obj, &"statements".into(), &stmts_val).map_err(|e| e)?; | |
| js_sys::Reflect::set(&obj, &"comments".into(), &comments_val).map_err(|e| e)?; |
|
Hi @elliots thanks for your contribution. Could you help handle those review comments and also the pre-commit issue? I'm happy to merge this after that. |
Why
Allows access to comments, and provides correct spans (so i can find what the comments are referring to)
How
Exposes the parseWithComments function, and uses tokenize_with_location
NOTE: claude did this. I'm using it, so it seemingly works ok, but just a heads-up.