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.

Sign up for GitHub

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

Jump to bottom

Switch to graphemer to split strings #51

Merged
merged 3 commits into from
Aug 4, 2023
Merged

Switch to graphemer to split strings #51

merged 3 commits into from
Aug 4, 2023

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Aug 4, 2023 •
edited
Loading

I had a problem with consuming the ESM module in vitest.
The resulting bundle had a import { split } from "lodash", which does not work because lodash is not an ESM module.
There is an alternative lodash-es which is an ESM module, but then using that would break some CJS environments.

lodash's split function notably doesn't handle well some emoji graphemes, so instead this switches to graphemer

fixes element-hq/compound#196

@sandhose
Switch to lodash-es and embed it in the bundle
ebb6855 
This switches to lodash-es to properly use a tree-shakable version of lodash.
It also makes sure that we're including the lodash methods we use in the bundle instead of having it as a library dependency.
@sandhose sandhose requested a review from a team as a code owner August 4, 2023 13:03
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 4, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d7afdea
Status: ✅  Deploy successful!
Preview URL: https://67502f1d.compound-web.pages.dev
Branch Preview URL: https://quenting-lodash-es.compound-web.pages.dev

View logs

@t3chguy
Copy link
Member

t3chguy commented Aug 4, 2023

image

Lodash currently makes up a not insignificant amount of the react SDK main bundle. This sounds like it'll double that.

t3chguy
t3chguy reviewed Aug 4, 2023
View reviewed changes
src/utils/string.ts Outdated
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { split } from "lodash";
import { split } from "lodash-es";
Copy link
Member

Choose a reason for hiding this comment

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

Also worth pointing out EW hit bugs in Lodash's split grapheme splitting, we had to switch to graphemer

Copy link
Member

@t3chguy t3chguy Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

See matrix-org/matrix-react-sdk#10976 + matrix-org/matrix-react-sdk#11037

Copy link
Member

Choose a reason for hiding this comment

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

beaugunderson/emoji-aware#25 & lodash/lodash#5652 being related issues about this bug

Copy link
Member

Choose a reason for hiding this comment

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

Have opened element-hq/compound#196 to track this

@sandhose
Copy link
Member Author

sandhose commented Aug 4, 2023

Not that much, because this tree-shakes correctly, so only the methods we use (in this case, _.split) are included in the bundle.

Library size with lodash-es not marked as external:

dist/style.css        20.06 kB │ gzip: 3.39 kB
dist/compound-web.js  21.02 kB │ gzip: 6.36 kB
dist/compound-web.cjs  16.59 kB │ gzip: 5.71 kB

and with it marked as external:

dist/style.css        20.06 kB │ gzip: 3.39 kB
dist/compound-web.js  16.40 kB │ gzip: 4.68 kB
dist/compound-web.cjs  13.03 kB │ gzip: 4.19 kB

So there is a 1.5kB difference gzipped

@t3chguy t3chguy mentioned this pull request Aug 4, 2023
Closed
@t3chguy
Copy link
Member

t3chguy commented Aug 4, 2023

So there is a 1.5kB difference gzipped

Otherwise known as 36%...

@sandhose
Copy link
Member Author

sandhose commented Aug 4, 2023

Alright fair, switching to graphemer then

@sandhose sandhose changed the title Switch to lodash-es and embed it in the bundle Switch to graphemer to split strings Aug 4, 2023
t3chguy
t3chguy reviewed Aug 4, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
t3chguy
t3chguy approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

:shipit: bonus points for the flag test

@sandhose sandhose merged commit e54e32b into main Aug 4, 2023
6 of 7 checks passed
@sandhose sandhose deleted the quenting/lodash-es branch August 4, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t3chguy t3chguy t3chguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avatar initial letter wrong for flag emoji
2 participants
@sandhose @t3chguy