Skip to content

fix: Template export { x } stuck in infinite loop#15534

Merged
liuxingbaoyu merged 1 commit intobabel:mainfrom
liuxingbaoyu:fix-tpl-export
May 10, 2023
Merged

fix: Template export { x } stuck in infinite loop#15534
liuxingbaoyu merged 1 commit intobabel:mainfrom
liuxingbaoyu:fix-tpl-export

Conversation

@liuxingbaoyu
Copy link
Member

@liuxingbaoyu liuxingbaoyu commented Mar 31, 2023

Q                       A
Fixed Issues? Fixes #15525
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?
Documentation PR Link
Any Dependency Changes?
License MIT

Thanks @await-ovo!

@liuxingbaoyu liuxingbaoyu added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: template labels Mar 31, 2023
@babel-bot
Copy link
Collaborator

babel-bot commented Mar 31, 2023

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54203/

let nameSet: Set<string>;
let metadata;
let prefix = "";
let prefix = "BABEL_TPL$";
Copy link
Member

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?

Copy link
Member Author

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));
Copy link
Member

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);

Copy link
Member Author

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.

Copy link
Member

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).

Copy link
Member Author

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

@liuxingbaoyu liuxingbaoyu assigned JLHwung and unassigned JLHwung Apr 14, 2023
@liuxingbaoyu liuxingbaoyu requested a review from JLHwung April 14, 2023 06:08
Copy link
Contributor

@JLHwung JLHwung left a 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.

@liuxingbaoyu liuxingbaoyu merged commit f347e9f into babel:main May 10, 2023
@liuxingbaoyu liuxingbaoyu deleted the fix-tpl-export branch May 10, 2023 13:46
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: template PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: template.ast goes endless loop

5 participants