-
Notifications
You must be signed in to change notification settings - Fork 140
Add AST-based route harvesting for Node.js variable paths #2512
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -473,6 +473,16 @@ func (e *RouteExtractor) scanFile(filePath string) error { | |
| return err | ||
| } | ||
|
|
||
| // Fallback pass: resolve routes whose path comes from a variable or a | ||
| // template literal, which the line-based regex harvesters cannot match. | ||
| src, ok, err := readJSFileForScan(filePath) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every file is read and parsed twice. The regex pass above streams line-by-line; this AST pass then reads the whole file into a string (up to |
||
| if err != nil { | ||
| return err | ||
| } | ||
| if ok { | ||
| e.routes = append(e.routes, e.resolveASTRoutes(filePath, src)...) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript files pay full cost for nothing. |
||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
||
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.
New heavyweight dependency.
dop251/goja(a full JS interpreter, plus transitivego-sourcemap/sourcemap) is pulled in only for its parser/ast, and pinned to an untagged pseudo-version. Worth a sentence justifying the weight vs. a lighter JS parser. Separately,go.sumbumpsMasterminds/semver/v33.4.0 → 3.5.0 — looks like an unrelatedgo mod tidyside effect; confirm it's intended here.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.
Yeah this is a good point, if implementing this means we have to bring in a full JS interpreter, perhaps we should wait until a customer reports an issue with our existing route parsing. So far there hasn't been any requests.