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
fix: Template export { x }
stuck in infinite loop
#15534
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54203/ |
37b4f08
to
0597df6
Compare
let nameSet: Set<string>; | ||
let metadata; | ||
let prefix = ""; | ||
let prefix = "BABEL_TPL$"; |
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.
If we are always adding enough $
s anyway, why do we need a more complex prefix?
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.
Just to reduce the possibility of duplication.
: [], | ||
), | ||
prefix = "$$" + prefix; | ||
} while (raw.includes(prefix)); |
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 trying until we add enough $
s, can we instead do this and only compute it once?
let $count = 0;
const re = /\$+(?=\d)/g;
let m;
while (m = re.exec(tpl)) $count = Math.max($count, m[0].length);
const prefix = "$".repeat($count + 1);
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.
I guess includes()
would be a bit faster? Of course it doesn't matter.
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.
The difference is that with includes we iterate over the string n times (where "n" is the final number of times we have to expand the prefix), while with the regexp loop only once (because the regexp alwags start after the previous match, and matches prefixes of any length).
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.
It seems that only in the last case (very long code and $$$$$$0
) the regex is slightly faster, while in other cases includes()
is faster.
It should be faster to use BABEL_TPL$
as per the current code.
perf
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.
I am not very familiar with babel template, rubberstamp lgtm.
Thanks @await-ovo!