Skip to content
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

Merged
merged 1 commit into from May 10, 2023

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
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
54 checks passed
@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