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: Replace custom repeatString function with repeat() #2820

Merged
merged 1 commit into from
May 30, 2023

Conversation

DreierF
Copy link
Contributor

@DreierF DreierF commented May 29, 2023

Marked version:

5.0.3

Description

While working on #2805 I noticed that there are a few repeatString functions that could trivially be replaced with String.proptotype.repeat(), which is already used in some other places in the code anyway.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
    • New em_list_links
    • New em_and_reflinks
    • CommonMark Emphasis and strong emphasis
    • CommonMark Links
    • CommonMark HTML blocks
    • Original markdown_documentation_basics
    • Original markdown_documentation_syntax
    • GFM HTML blocks
    • GFM Emphasis and strong emphasis
    • GFM Links
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

@vercel
Copy link

vercel bot commented May 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marked-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 30, 2023 5:18am

Copy link
Member

@UziTech UziTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

@calculuschild
Copy link
Contributor

Looks like the old function was there because at the time .repeat() wasn't standard JS. That function was taken from StackOverflow in a thread asking about the fastest method. Just out of curiosity, is there any benchmark comparing the speed of that approach vs the built-in function?

@DreierF
Copy link
Contributor Author

DreierF commented May 30, 2023

I didn't perform any benchmark, but I would assume that the built-in functionality should be at least as fast as the custom implementation. Actually I would expect it to be faster as a lot of the string allocation+concatenation can be done in one go in the native implementation.

Update: Did a benchmark now out of curiosity. Result: It depends 😅 If you count the cost of creating the function it the old version is always slower than repeat() on my M1 Max (slower on mobile). If you don't it depends on the browser. It's faster in Safari and Firefox even if you don't count the function creation and a tiny bit slower in Chrome (not significantly 1%).

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@davisjam
Copy link
Contributor

Should be same-or-faster unless we're repeating really small strings, in which case the implementation shouldn't matter anyway. But it's nice to use the built-in language feature.

Do all marked-supported language runtimes now support repeat?

@UziTech UziTech changed the title Replace custom repeatString function with repeat() fix: Replace custom repeatString function with repeat() May 30, 2023
@UziTech UziTech merged commit 72ee2d6 into markedjs:master May 30, 2023
@styfle
Copy link
Member

styfle commented May 30, 2023

We don't have a list of supported runtimes outside of Node.js as far I know.

We support Node.js 18 and newer since Marked 5.0.0 (see #2767)

github-actions bot pushed a commit that referenced this pull request May 30, 2023
## [5.0.4](v5.0.3...v5.0.4) (2023-05-30)

### Bug Fixes

* Add Unicode punctuations ([#2811](#2811)) ([b213f02](b213f02))
* fix hr after list ([#2809](#2809)) ([efc40df](efc40df))
* Replace custom `repeatString` function with `repeat()` ([#2820](#2820)) ([72ee2d6](72ee2d6))
@DreierF DreierF deleted the repeat branch May 31, 2023 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants