-
-
Notifications
You must be signed in to change notification settings - Fork 140
Fix(javascript): Handle namespace imports correctly #729
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
Conversation
|
I also want to add a commit that improves the error handling in the JavaScript and TypeScript transformers. Previously, when a transformation failed, a generic error was thrown, making it difficult to debug issues. This PR introduces the following changes to provide more informative error messages:
These changes will make it easier for users to identify and fix errors in their code. |
|
Tried out the before and after locally, works fine for me and the error is printed nicely. |
| (each [child (n:iter_children)] | ||
| (traverse child))))) | ||
| (traverse node) | ||
| found) |
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.
Instead of using a var and mutating it I'm pretty sure this can be implemented with an if statement and returning the result of (traverse node) - inside the recursive function we can return at the point you (set found n) . We don't actually use a return keyword though which breaks out of the control flow, instead we should have two steps.
In the let block we can do the lookup, if we find a value, we end the recursion and return the found value in the tail position. I think we also have to check the result of each sub-call to traverse in that each so we return if something is returned from the child call.
I think this current code actually iterates more than it needs to and doesn't exit early when it finds a match? Just a small optimisation but I think this can be written in a true recursive (although maybe not tail recursive because of the tree nature, that would require more state to be passed along in the recursion which I don't care about so much) function that would be easier to follow and perform less work when it its an error.
I could be wrong about this though because I've been pretty sick all week and I'm still kind of out of it today but I wanted to get a response to you 😅
If you disagree or would rather not try to implement this in a recursive way, please let me know, I'm open to being swayed.
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.
Thank you for sharing your thoughts! Unfortunately, I don't have time to look into it right now, but I will as soon as I can.
I hope you feel better soon!
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.
Here is a corrected version of the original recursive implementation, which did not exit early and performed unnecessary work.
My change to an iterative approach was partly to fix that, as the while (and (not found) ...) loop will stop as soon as a forbidden keyword is found.
While a pure recursive function without mutation would be more idiomatic and arguably cleaner, I chose the iterative approach for one main reason: to prevent stack overflows.
Since we're traversing a syntax tree that could be very deeply nested in complex source files, a recursive solution runs the risk of hitting Lua's stack limit. The iterative approach with an explicit stack avoids this problem entirely, making the transformer more robust.
So, while I agree that a recursive solution can be more elegant, I believe the iterative one is a safer choice here given the unpredictable depth of the trees we might be parsing.
I'm happy to discuss this further if you still think the recursive approach is the way to go.
|
🚀 |
This change addresses a bug in how JavaScript namespace imports (e.g., import * as name from "module") are transformed.
The previous implementation incorrectly assumed the structure of the AST node for a namespace import, leading to errors when trying to get the alias. This has been corrected by using tsc.find-child-by-type to reliably find the identifier node.
This ensures that namespace imports are correctly converted to require statements.