[No QA] Make the html-entities worklet patch idempotent - #98395
Conversation
The patch added "worklet"; as a new line above "use strict";. Because that was a pure insertion, its context still matched once applied, so patch-package never recognised the patch as already applied and inserted another copy on every run. Anyone who ran npm install more than once accumulated duplicate directives in node_modules, which then leaked into the bundled GitHub Actions and failed Validate Github Actions against CI's clean install, on PRs that had nothing to do with this patch. Prepend the directive to the existing "use strict"; line instead. The removal context then stops matching once applied, so patch-package detects the patch as already applied and leaves the file alone. Both remain directives: a block comment does not interrupt the directive prologue, so "use strict" still takes effect and "worklet" is still first. Verified against a pristine html-entities@2.5.3 that three consecutive patch-package runs leave exactly one directive, exit 0, and emit no warnings (applyPatches.sh treats any warning as a failure).
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@NicolasBonet Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.4.53-0 🚀
|
3 participants
Explanation of Change
patches/html-entities/html-entities+2.5.3+001+use-worklet.patchwas not idempotent, so it re-applied on everynpm installand accumulated duplicate"worklet";directives innode_modules. This is a problem because it corruptsnpm run gh-actions-buildsuch that there's a duplicated worklet line for everynpm ithat you run between builds.The patch added the directive as a new line above
"use strict";:Because that is a pure insertion, the context line it anchors on (
"use strict";var __assign=...) still matches after the patch has been applied — just shifted down by one line. patch-package applies it again at the new offset instead of recognising it as already applied, and reports success while doing so. Every subsequentnpm installadds another copy.The fix prepends the directive to the existing
"use strict";line rather than adding a line of its own:The patch now modifies a line instead of only inserting one, so once applied its removal context no longer matches and patch-package correctly detects the patch as already applied and leaves the file alone.
The comment changes from
//to/* */because the directive now shares a line with the code that follows it.Both remain directives. A block comment does not interrupt the directive prologue, so
"use strict"still takes effect and"worklet"is still the first directive — which is whatreact-native-reanimatedrequires.Fixed Issues
$ n/a - annoying development quirk.
Tests
npm installseveral times so the patch would previously have applied more than oncenpm run gh-actions-buildgrep -c 'must be a worklet' .github/actions/javascript/proposalPoliceComment/index.jsand verify it prints1Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A