URL: Remove all of Lodash from the package #41687
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Performance
tyxla
requested review from
gziolo,
swissspidy,
andrewserong,
ntsekouras,
fabiankaegy and
a team
|
Size Change: +1.63 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Noting that the size stats results are not surprising; |
adamziel
reviewed
Jun 13, 2022
packages/url/src/clean-for-slug.js
Outdated
| .toLowerCase(), | ||
| '-' | ||
| ); | ||
| return removeAccents( string ) |
There was a problem hiding this comment.
Would this work too, @tyxla ?
const str = "Crème Brulée"
str.normalize("NFD").replace(/[\u0300-\u036f]/g, "")
> "Creme Brulee"Found it on StackOverflow – perhaps we could do without the remove-accents dependency entirely? I saw it was based on a characters map so perhaps str.normalize would give us an even better coverage for free?
There was a problem hiding this comment.
Thanks for asking @adamziel, it's actually a great question!
I'd be happy to go with that, it's always ideal to not rely on another package. The thing is, that solution works for most of the characters, but from what I've seen, it doesn't cover them all, or at least, doesn't cover as many as the package I've used covers.
Try this in your browser console - I've built it from the character map of the package:
"ÀÁÂÃÄÅẤẮẲẴẶÆẦẰȂÇḈÈÉÊËẾḖỀḔḜȆÌÍÎÏḮȊÐÑÒÓÔÕÖØỐṌṒȎÙÚÛÜÝàáâãäåấắẳẵặæầằȃçḉèéêëếḗềḕḝȇìíîïḯȋðñòóôõöøốṍṓȏùúûüýÿĀāĂ㥹ĆćĈĉĊċČčC̆c̆ĎďĐđĒēĔĕĖėĘęĚěĜǴĝǵĞğĠġĢģĤĥĦħḪḫĨĩĪīĬĭĮįİıIJijĴĵĶķḰḱK̆k̆ĹĺĻļĽľĿŀŁłḾḿM̆m̆ŃńŅņŇňʼnN̆n̆ŌōŎŏŐőŒœP̆p̆ŔŕŖŗŘřR̆r̆ȒȓŚśŜŝŞȘșşŠšŢţțȚŤťŦŧT̆t̆ŨũŪūŬŭŮůŰűŲųȖȗV̆v̆ŴŵẂẃX̆x̆ŶŷŸY̆y̆ŹźŻżŽžſƒƠơƯưǍǎǏǐǑǒǓǔǕǖǗǘǙǚǛǜỨứṸṹǺǻǼǽǾǿÞþṔṕṤṥX́x́ЃѓЌќA̋a̋E̋e̋I̋i̋ǸǹỒồṐṑỪừẀẁỲỳȀȁȄȅȈȉȌȍȐȑȔȕB̌b̌Č̣č̣Ê̌ê̌F̌f̌ǦǧȞȟJ̌ǰǨǩM̌m̌P̌p̌Q̌q̌Ř̩ř̩ṦṧV̌v̌W̌w̌X̌x̌Y̌y̌A̧a̧B̧b̧ḐḑȨȩƐ̧ɛ̧ḨḩI̧i̧Ɨ̧ɨ̧M̧m̧O̧o̧Q̧q̧U̧u̧X̧x̧Z̧z̧".normalize("NFD").replace(/[\u0300-\u036f]/g, "")
and you'll get:
AAAAAAAAAAAÆAAACCEEEEEEEEEEIIIIIIÐNOOOOOØOOOOUUUUYaaaaaaaaaaaæaaacceeeeeeeeeeiiiiiiðnoooooøoooouuuuyyAaAaAaCcCcCcCcCcDdĐđEeEeEeEeEeGGggGgGgGgHhĦħHhIiIiIiIiIıIJijJjKkKkKkLlLlLlĿŀŁłMmMmNnNnNnʼnNnOoOoOoŒœPpRrRrRrRrRrSsSsSSssSsTttTTtŦŧTtUuUuUuUuUuUuUuVvWwWwXxYyYYyZzZzZzſƒOoUuAaIiOoUuUuUuUuUuUuUuAaÆæØøÞþPpSsXxГгКкAaEeIiNnOoOoUuWwYyAaEeIiOoRrUuBbCcEeFfGgHhJjKkMmPpQqRrSsVvWwXxYyAaBbDdEeƐɛHhIiƗɨMmOoQqUuXxZz
where while the result is satisfactory, you will notice that there are quite a few that actually remain unchanged.
For the library above, we also used the corresponding WP function originally when compiling the character map, so this also gets us closer to a parity with the WordPress backend - a nice side benefit IMHO.
What do you think?
There was a problem hiding this comment.
Nice test!
For the library above, we also used the corresponding WP function originally when compiling the character map, so this also gets us closer to a parity with the WordPress backend - a nice side benefit IMHO.
Alright, let's go with the package 👍
adamziel
reviewed
Jun 13, 2022
Co-authored-by: Adam Zielinski <[email protected]>
adamziel
approved these changes
Jun 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR removes all of the Lodash from the
@wordpress/urlpackage. There are just a few usages and conversion is pretty straightforward.Fixes #39495.
Why?
Lodash is a major part of the
@wordpress/urlpackage - over 94%. By removing it, we're drastically reducing the bundle size of that package and eventually all of the packages that depend on it (@wordpress/api-fetchbeing the obvious one to call out here - see #39495). It's one of the packages that have a utility on the frontend, and by keeping it small, we reduce the bytes that have to be downloaded for the end users of sites that use that package on the front end. Last, but not least, all the projects that use this package will also benefit from the smaller bundle size.Furthermore, Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:
@wordpress/api-fetchpackage haslodashas a dependency #39495How?
We have to deal essentially with 3 methods, and each of them is handled specifically:
deburr()The most complex one. Used to essentially convert accented characters to their non-accented versions. Luckily, I wrote a clone of WordPress core's PHP
remove_accents()a while ago, and wrapped it as a package. Simple as it is, it does all the conversion for us at little cost. It also comes with TS definitions.trim()Used to remove all leading and trailing dashes. That's straightforward enough to replace with another simple
replacecall.every()Used only in tests to verify all correct emails are considered correct emails, and all incorrect emails are considered incorrect emails by the
isEmail()function.However, these tests currently have a bug.
every()will returnfalseif even one of these isfalse, so the assertion that all of them are falsy is incorrect. This PR uses the opportunity to rewrite the incorrect assertions to separate tests (usingit.each) and that way each of the emails is a separate test case. That allows us to catch the failing one that was being hidden under the ambiguousevery()call. You'll notice that I'm moving the1234567890123456789012345678901234567890123456789012345678901234+x@wordpress.orgemail address that is actually valid according to the regex to the group of emails that are considered valid. The idea is to make tests pass since the tests were previously wrong, and not introduce any functional changes to theisEmail()function, since those are unrelated to the purpose of this PR.If y'all would prefer that, I'm happy to follow up and introduce this backwards compatibility breaking change to
isEmail()and provide a limit to the "local-part" of the email address so that email is considered invalid. However, I personally didn't see anything about a limit of the "local-part" of the email address in the IME RFC (RFC5322), so my personal preference would be to not touch the function and just adapt the tests, which I did in this PR.Testing Instructions
Verify all tests pass:
npm run test-unit packages/url