[$500] ExpensiMark: large bundle size

[tomekzaw]

Problem
A simple script that uses ExpensiMark library to parse Markdown into HTML has a bundle size of 336 kB (or 184 kB minified) which is quite big.

Details
The file is large because it contains mappings of all HTML entities (from _.escape) and countries, states, currencies etc. from CONST.jsx.

Solution
Investigate if it's possible to minimize the bundle size.

Steps to reproduce

index.js

import ExpensiMark from "expensify-common/lib/ExpensiMark";

const input = "Hello, *world*!";
const parser = new ExpensiMark();
const output = parser.replace(input);

console.log(JSON.stringify(input));
console.log(JSON.stringify(output));

esbuild index.js --bundle --minify --outfile=out.js

out.js: https://pastebin.swmansion.com/?9798f33833cc33b6#DRgiwxs8ujteamHaGDioSzZVZ83nuWUim3QabJ5PUYed

Issue Owner

Current Issue Owner: @Skalakid

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0155a63860a646579c
• Upwork Job ID: 1740457868647858176
• Last Price Increase: 2024-01-04
• Automatic offers:
• fedirjh | Reviewer | 28090032
• aswin-s | Contributor | 28099676

[melvin-bot]

Triggered auto assignment to @abekkala (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[roryabraham]

Co-assigning myself to supervise, but this will be worked on by Michał Skałka from SWM, who will be invoicing us separately

[roryabraham]

Maybe the first and easiest steps here are to:

1. Port Expensimark to typescript and stop using underscore – that would likely help reduce the file size
2. Split up the big CONST export and separately export/import only the pieces that we need in Expensimark

[Skalakid]

Hi I’m Michał from Software Mansion, an expert agency, and I’d like to work on this issue

[roryabraham]

No update from me, but @Skalakid keep me posted if you're starting to work on this

[roryabraham]

not urgent, going to make @Skalakid the issue owner here

[melvin-bot]

This issue has not been updated in over 15 days. @Skalakid, @roryabraham eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[roryabraham]

Moving this back to weekly. We are hoping to launch this feature in January, so we should start making moves here too. @Skalakid is going to be focused on inline video, so we should find someone else to tag in on this.

[tomekzaw]

Would be great to have this for V1 but currently JS bundle of Live Markdown parser is 188 kB so it's not a blocker I guess.

[roryabraham]

I'm going to make this external and accept proposals for open-source contributors to work on this.

[roryabraham]

@kidroca I like the idea to just align the underscore version used in expensify-common and E/App. That seems like a very easy way to get a 12% bundle size reduction without much effort.

[melvin-bot]

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[kidroca]

@kidroca I like the idea to just align the underscore version used in expensify-common and E/App. That seems like a very easy way to get a 12% bundle size reduction without much effort.

It appears that E/App is already aligned with expensify-common in terms of the underscore version used:

• E/App employs ^1.13.1, which, due to the caret (^) symbol, automatically resolves to the latest patch version within the minor version 1.13. As of now, this is 1.13.6.
  (You can see which version of underscore a project is using by running npm why underscore)
• expensify-common is explicitly using version 1.13.6.

Given this setup, for broader compatibility with other projects, we could consider moving underscore to peerDependencies in expensify-common. This move would allow us to declare a wider range of acceptable versions, effectively bridging any version gaps among different projects.

Alternatively, if placing underscore under peerDependencies poses challenges, we could simply adjust the version specification in expensify-common to use the caret (^) prefix. This change would ensure that any patch within the current minor version (1.13.x) is compatible, thus maintaining flexibility for patch updates without necessitating major version changes. Such a strategy would align with semver principles and could facilitate easier maintenance and compatibility across projects.

[aswin-s]

@roryabraham Also not sure why we overlooked the removal of String.prototype.replaceAll. That amounts to 30KB (16% savings). Seems like that's more relevant than underscore since it is being shared with other packages.

https://bundlephobia.com/package/[email protected]

[kidroca]

I agree with @aswin-s's suggestion regarding the removal of the String.prototype.replaceAll polyfill. This change is straightforward and indeed offers a significant reduction in the bundle size

Removing the polyfill impacts the backward compatibility and potential technical debt for legacy systems. It might be beneficial to conduct a quick survey or analysis to determine how many such legacy projects are dependent on expensify-common and their current Node.js versions.

I am generally in favor of removing the polyfill, as it primarily affects projects running on very old versions of Node.js (older than 15), which are likely to be a minority if any

[roryabraham]

@aswin-s sorry about that, I do agree with removing the replaceAll polyfill.

@aswin-s I'll co-assign this issue to you to open a PR to remove that polyfill, thanks.

[melvin-bot]

📣 @aswin-s 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[aswin-s]

@roryabraham Not sure if this is the right place to ask, but since it also impacts the bundle size of ExpensiMark, I'll ask anyway.

We have a really large regex to match valid TLDs. Although it doesn’t significantly impact browser or desktop environments, it takes a considerable amount of time to execute on mobile devices. On Android devices, the TLD matching, which is part of markdown parsing, takes around 3 seconds to execute, thereby blocking the UI. The execution time increases exponentially with size of the matching text. I was wondering if there is a specific reason for having such stringent TLD matching, or if it's just been retained for historical reasons. Simplifying this to a shorter regex pattern that checks the validity of the TLD pattern, rather than checking explicit domains, would reduce the bundle size by around 10KB and significantly increase regex execution speed. I would be interested in hearing your thoughts on this.

Came across this while I was investigating performance issues in parsing markdown content in this issue.

[roryabraham]

@abekkala pulling you back in to help manage payments here. @kidroca invoices us separately but I think we may need to create and upwork job for @aswin-s

Edit: already done: #31183 (comment)

[aswin-s]

PR was merged on Jan 15th. This one is ready for payment.

[abekkala]

@roryabraham I want to make sure I'm looking at the Correct PR.
The one above has not merged to production yet, correct?
The payment countdown starts after it's merged to production and there are no regressions.

@aswin-s The PR above Expensify/expensify-common#632 is not showing merged to prod yet.

once that happens the payment title for this GH will auto-update with the payment date

[aswin-s]

@abekkala PR is merged to master in expensify-common repository. Melvin seems to be tracking PRs from Expensify/App repository only.

@roryabraham Could you please confirm?

[roryabraham]

Ah, sorry for the confusion here. @aswin-s can you please open an E/App PR to upgrade the expensify-common version we're using (otherwise the PR won't take effect). Then once that hit production we can issue payment.

[fedirjh]

This PR got superseded by #34801. Expensify-commons has got upgraded to a more recent commit than the one mentioned in this PR.

It seems that expensify-common was upgraded in #34801.

[aswin-s]

Ah, sorry for the confusion here. @aswin-s can you please open an E/App PR to upgrade the expensify-common version we're using (otherwise the PR won't take effect). Then once that hit production we can issue payment.

@abekkala @roryabraham Above PR has already hit PROD last week.

[roryabraham]

@abekkala can we please issue payment of $500 to @aswin-s for Expensify/expensify-common#632 ?

[abekkala]

@roryabraham yes, I can send an upwork offer to @aswin-s
Was there someone who did the PR review (that needs payment as well)? or was that done internally?

[roryabraham]

PRs for this issue were reviewed internally

[abekkala]

@aswin-s payment sent and contract ended - thank you!