fix(useClipboard): prevents fail in Safari for async operation - #5369
Conversation
| } | ||
|
|
||
| return { | ||
| copyPending, |
There was a problem hiding this comment.
this should be readonly:
| copyPending, | |
| copyPending: shallowReadonly(copyPending), |
| function createClipboardItem(value: ClipboardValue): ClipboardItem { | ||
| if (typeof value === 'string') { | ||
| text.value = value | ||
| return new ClipboardItem({ 'text/plain': value }) | ||
| } | ||
| else { | ||
| return new ClipboardItem({ | ||
| 'text/plain': value().then((resolvedText = '') => { | ||
| text.value = resolvedText | ||
| return new Blob([resolvedText], { type: 'text/plain' }) | ||
| }), | ||
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
To simplify this, we can wrap the string in a Promise, which reduces the amount of code. If it's a string, it becomes a Promise. If it's already a Promise, we just use it.
There was a problem hiding this comment.
not sure if we should do this. The check is cheaper than a promise
| expect(text.value).toBe('') | ||
| expect(copied.value).toBe(false) | ||
|
|
||
| await copy(async () => 'async text') |
There was a problem hiding this comment.
instead of an immediately resolved promise, can you pass one you manually resolve()? then assert that copyPending changes as expected
OrbisK
left a comment
There was a problem hiding this comment.
This looks overall good to me.
One thing we have to discuss might be how we want to handle multiple pending async copies.
should we abort? I think .write does not support AbortController afaik.
| } | ||
| catch { | ||
| useLegacy = true | ||
| copyPending.value = false |
|
@OrbisK Yeah, there's no support for abort, but we could use the pending state to avoid triggering copy again until the previous operation is done. |
I think we should not defer/dedupe it. I think copy should just do what it does. But I think we need to make sure that pending is true as long as all copies are not finished. copy(simpleAsynCopy) // sets pending to true - takes 10 seconds
copy("sync value") // sets pending to true and immediate to false, but async is still pending
copyPending.value // should be true, but might be false |
|
just like your regular clipboard, new copy actions should discard the old ones i think. const asyncValue = somehowGetAsyncValue("foo");
const syncValue = "bar";
const asyncValue2 = somehowGetAsyncValue("baz");
copy(asyncValue); // pending
copy(syncValue); // set clipboard to "bar", abandon the promise from before
copy(asyncValue2); // pending
// eventually, set clipboard to "baz" because the last promise resolved |
|
There may be a UX issue:
|
|
Love that we used a classic foobar action without knowing, ahahahahha |
|
i checked, the clipboard API writes the last resolved value. so lets just do the same here and not implement our own behaviours. this: const asyncValue = somehowGetAsyncValue("foo");
const syncValue = "bar";
const asyncValue2 = somehowGetAsyncValue("baz");
copy(asyncValue); // pending
copy(syncValue); // set clipboard to "bar"
// asyncValue resolved, set clipboard to "foo"
copy(asyncValue2); // pending
// eventually, set clipboard to "baz" because the last promise resolved |
Nice! We still have to implement the "dedupe" for the legacy api. It currently await the resolve, so it will use the latest resolved value. We should handle this and we are good to go. |
|
@OrbisK I think this last change should do the trick |
@vueuse/components
@vueuse/core
@vueuse/electron
@vueuse/firebase
@vueuse/integrations
@vueuse/math
@vueuse/metadata
@vueuse/nuxt
@vueuse/router
@vueuse/rxjs
@vueuse/shared
@vueuse/skills
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5369 +/- ##
==========================================
- Coverage 65.10% 65.07% -0.03%
==========================================
Files 346 346
Lines 8141 8163 +22
Branches 2508 2514 +6
==========================================
+ Hits 5300 5312 +12
- Misses 2313 2322 +9
- Partials 528 529 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
resolves #5368
Additional context
I've ensured compatibility with the current implementation to prevent breaking changes. The copy method still accepts an optional string, continuing to default from the source. Additionally, it now can also accept a function that returns a Promise resolving to a string. Since this operation can now be asynchronous, I've added a new
copyPendingboolean to enable showing a pending state when necessary.This is my first time contributing to Vueuse, so let me know if I'm missing anything. I'm not too familiar with the codebase.
This could be a first draft. We can improve readability as well, if needed.
We could also decide to create a completely separate composable.
Thanks again for taking the time to read this ❤️