perf: migrate to tinyclip from clipboardy#21975
Conversation
|
All contributors have signed the DCO. |
|
I have read the DCO document and I hereby sign the DCO. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21975 +/- ##
=======================================
Coverage 95.11% 95.11%
=======================================
Files 549 549
Lines 45644 45644
Branches 6528 6524 -4
=======================================
Hits 43413 43413
Misses 2101 2101
Partials 130 130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| */ | ||
|
|
||
| import clipboard from 'clipboardy'; | ||
| import * as clipboard from 'tinyclip'; |
There was a problem hiding this comment.
Looking at the following code, tinyclip only shells out to wl-copy, xsel, or xclip: https://github.com/tinylibs/tinyclip/blob/23e04472d30b7040c7a43fcd0caa64d8a93ad748/src/index.ts#L72-L96
clipboardy bundled an xsel fallback binary: https://github.com/sindresorhus/clipboardy/blob/003856640e971c53d0ce7731d0dd9baca9131ce4/readme.md?plain=1#L142
Will carbon-cli changelog with tinyclip fail in a minimal Linux or CI environment that does not already have a clipboard util installed?
There was a problem hiding this comment.
Yes that is one of the reasons why tinyclip is so much lighter, we think it's not the responsibility of such library to bundle this kind of fallback (that means as a windows user, you have to download 1MB of fallback you will never need). So in a minimal env it may indeed fail.
I understand if you don't want the change because of that, it's fair! That would require installing xclip first for example (which IMO is better): sudo apt-get install -y xclip && carbon-cli changelog
tay1orjones
left a comment
There was a problem hiding this comment.
Thanks for this! Looks good to me.
|
|
||
| if (response?.copy) { | ||
| clipboard.writeSync(changelog); | ||
| await clipboard.writeText(changelog); |
There was a problem hiding this comment.
This is guarded by noPrompt, so no risk here really.
|
|
||
| if (copy) { | ||
| clipboard.writeSync(testFile); | ||
| await clipboard.writeText(testFile); |
There was a problem hiding this comment.
This is an old test generation script that was primarily used when we were migrating from enzyme to react-testing-library (rtl). I don't think it's used much anymore, and if it is, it's a local dev environment where there's no risk.
adamalston
left a comment
There was a problem hiding this comment.
Could you address the merge conflicts?
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
e21e19a
Tinyclip is really lighter than clipboardy. This PR was made as part of the e18e initiative.
Changelog
Changed
clipboardydependency fortinyclipTesting / Reviewing
Run the changelog command and the build-test-rtl task.
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
More details can be found in the pull request guide