Lodash: Refactor away from _.delay()
#42966
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
|
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
|
Hey there 👋 I will check this PR, I'll unassign a few people from the reviewers list. |
Thanks a bunch, @geriux 🙌 I've fixed a couple of failing tests in the meantime. TIL about our |
geriux
approved these changes
Aug 4, 2022
|
Thanks a ton for the thorough testing @geriux 🙌 |
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 the
_.delay()usage completely and deprecates the function. Prior usage is only in RN components.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?
_.delay()is essentiallysetTimeout()with a few additional checks - ensuring that the callback is actually callable, and that the timeout duration is actually a number. None of these are necessary for our use cases, so replacing withsetTimeout()is just fine.This also unveiled an issue with the previous implementation that used
delay()- we weren't cleaning the timeouts up when components unmount, and that's bad for a couple of reasons:So in this PR we're also using the opportunity to fix that and clean up all the timeouts on unmount.
Testing Instructions
Inserter: Follow testing instructions from [RNMobile] Improve block inserter accessibility #25549 and verify they still work.MediaUpload: Follow testing instructions from [RNMobile] Simplify media insertion flow Part 2 - media upload #29547 and verify they still work.ColumnsEdit: Follow testing instructions from [RNMobile] Support percentage in columns #23828 and verify they still work.LinkPickerScreen: Follow testing instructions from [RNMobile] Upgrade to RN 0.66 #36328 (the Link Picker section) and verify they still work.npm run native test -- --maxWorkers=2 --cacheDirectory="$HOME/.jest-cache"