Lodash: Remove completely from @wordpress/docgen package
#43100
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
sgomes,
aristath,
gziolo,
swissspidy,
jorgefilipecosta,
chad1008,
andrewserong,
ntsekouras and
fabiankaegy
|
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
tyxla
force-pushed
the
remove/lodash-docgen
branch
from
9cd9025 to
c5c9cca
Compare
sgomes
reviewed
Aug 10, 2022
| @@ -31,7 +26,7 @@ module.exports = ( token ) => { | |||
| name = t.declaration.left.name; | |||
| break; | |||
| default: | |||
| name = get( t.declaration, [ 'id', 'name' ], '*default*' ); | |||
| name = t.declaration.id?.name ?? '*default*'; | |||
There was a problem hiding this comment.
Adding a note that ?. + ?? aren't strictly equivalent to get(), in that the former check for a value being nullish (null or undefined), whereas the latter checks for it being undefined only.
That said, this looks to still do the right thing in this case, as we wouldn't expect for null to be a correct value.
There was a problem hiding this comment.
I confirm this is the intention. IIUC we can expect undefined but not null there.
Comment on lines
+152
to
+153
| description: doc?.description ?? UNDOCUMENTED, | ||
| tags: doc?.tags ?? [], |
There was a problem hiding this comment.
Is null a valid value that we want to preserve in either of these cases, or do we always want to replace it with the default value? The lack of types makes this a bit tricky to review without more context 😕
There was a problem hiding this comment.
I don't see how those could be null TBH, so I'm not concerned about this.
| @@ -14,7 +9,10 @@ const { last } = require( 'lodash' ); | |||
| module.exports = ( declaration ) => { | |||
| let comments; | |||
| if ( declaration.leadingComments ) { | |||
| const lastComment = last( declaration.leadingComments ); | |||
| const lastComment = | |||
| declaration.leadingComments[ | |||
There was a problem hiding this comment.
If declaration.leadingComments is a plain array, then declaration.leadingComments.at(-1) would also work, assuming that the transpilation infrastructure supports it. But it may be safer to leave it as-is, and it's perfectly readable this way as well.
There was a problem hiding this comment.
I previously tried it, and it broke 😉 Seems like it's not supported.
|
Thank you for working on this, @tyxla! 🙌 |
|
Thanks for taking a look @sgomes 🙌 |
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/docgenpackage, including thelodashdependency altogether. There are just a few usages and conversion is pretty straightforward.Why?
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're dealing with straightforwardly replacing a few methods, namely:
getWe're using optional chaining and nullish coalescing.
lastStraightforward to replace with
x[ x.length - 1 ]on the target array.Testing Instructions
npx docgen packages/block-library/src/index.jsornpx docgen packages/block-editor/src/index.jsnpm run test:unit packages/docgen