-
Notifications
You must be signed in to change notification settings - Fork 2
Fix blade support #71
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
| } | ||
|
|
||
| /** | ||
| * Gets the file extension from a filename. |
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 keeping the code well-organized 🎉
|
@erikyo Happy to help. Thank you for reviewing so quickly! |
|
Thanks to you @sidp! I took the opportunity to fix an issue that arose with the tree-sitter update... in short, with the latest version (> 22) it seems that when using parser.parse some files that are too large return an error like this I am investigating why, and in the meantime I wrapped that function inside a try catch (#73), but I would like to understand in which cases the 'parse' function fails |
|
I see! It seems like const fileSize = Buffer.byteLength(sourceCode, "utf8");
const maxFileSize = 1024 * 128;
if (fileSize > maxFileSize) {
console.error(`File size of ${filepath} exceeds ${maxFileSize / 1024}KB limit.`);
return;
}
const bufferSize = Math.min(fileSize, maxFileSize) + 32; // 32 bytes of padding
// parse the file
const tree = parser.parse(sourceCode, undefined, {
bufferSize,
}); |
|
Nice catch! Can you add your code to PR #73? One idea: could we check if the file exceeds the limit and in that case make the buffer size dynamic? That way, for files that exceed the limit, we could still attempt to parse them by setting the buffer size to match the actual file size (plus padding). This way, larger files would not be completely blocked and the performance impact would only occur when actually needed. Something like: const fileSize = Buffer.byteLength(sourceCode, "utf8");
let bufferSize = 1024 * 128;
if (fileSize > bufferSize) {
console.warn(`File size of ${filepath} exceeds ${bufferSize / 1024}KB, using dynamic buffer size.`);
bufferSize = fileSize + 32; // Dynamic buffer for large files
}
const tree = parser.parse(sourceCode, undefined, {
bufferSize,
});What do you think of this approach? |
|
Sounds reasonable to me! This would however allow the buffer size to be set to very high numbers. Perhaps it makes sense to have a hard cap somewhere? V8 has a 1 GB limit on strings so I think that this would otherwise be the limiting factor. I would also keep 32 KB as the default to not change the memory usage under normal conditions. I can't push to the 1.6.2 branch but I can make a PR to it with these changes 😊 |
|
yep sorry, create a new PR and then i'll take care about merging into the current dev branch. thanks again!
Good point about the potential memory issues! A hard cap makes sense to prevent excessive memory usage, What do you think about 100MB as a hard cap? Or we should use totalmem and set it dynamically? |
This PR fixes support for blade templates, as reported in #70.
The problem:
Blade templates are parsed with
tree-sitter-php. The parser requires code to be within opening and closing<?php,?>tags. Blade doesn't use these, so the parser was not able to extract strings from blade files.The solution:
Use
tree-sitter-phpin thephp_onlyparser mode, which parses top-level statements without opening and closing tags. This mode is not available in the version oftree-sitter-phpthat was previously used, so it needed to be updated.Summary of changes:
tree-sitterdepencencies. I had to update all of them to resolve peer-dependency issues that arose from updating onlytree-sitter-php.{{}}echo statements and@phpdirectives.--skip-bladesetting is used.Tell me if you have any comments or questions!
close #70