promise cloneinto clarification #19125
Conversation
Preview URLsFlawsNone! 🎉 External URLsURL: No new external URLs (this comment was updated 2022-08-05 03:26:56.073439) |
|
|
||
| ```js | ||
| window.messenger.notify("Message from the page script!"); | ||
| ``` | ||
|
|
||
| In the special case of a Promise, which is not supported by [structured clone algorithm](/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm), the desired result can be achieved by using `window.Promise` instead of `Promise`, and then cloning the resolution value with `cloneInto` like this: |
There was a problem hiding this comment.
Could you move this after the next section and add a header such as "Promise cloning"?
There was a problem hiding this comment.
@Rob--W to clarify, you want it moved after the "Constructors from the page context" section?
There was a problem hiding this comment.
Yes, move it after that, and prepend a header.
| let result = { exampleKey: "exampleValue" }; | ||
| resolve(cloneInto(result, window)); | ||
| }); | ||
| // now promise can be passed to the web page. |
There was a problem hiding this comment.
@rpl Is this sufficiently clear, or do we need to elaborate on how the value is passed to the webpage?
There was a problem hiding this comment.
from my perspective, given that the preamble clearly states that a Promise can't be cloned, it should already make it clear that the resulting promise object from the snippet here should be passes as is to the webpage.
How that should be done seems like too much related to what the extension is doing (e.g. it could be passes as is as part of an event dispatched to the page, returned by a method exported to the webpage or set directly into a global using window.wrappedJSObject.someglobalname = promise and so I'm not sure it is good to point one out here, as it could be misleading.
Personally I'd like it with the current level of detail already provided by the current version of this PR.
If after these changes, this is still reported to us as not clear, then we may consider just create a complete example for the mdn/webextensions-examples repo and just link it here.
| @@ -188,12 +188,26 @@ window.wrappedJSObject.messenger = cloneInto( | |||
| {cloneFunctions: true}); | |||
| ``` | |||
|
|
|||
| Now page scripts will see a new property on the window, `messenger`, which has a function `notify()`: | |||
| Now page scripts sees a new property on the window, `messenger`, which has a function `notify()`: | |||
There was a problem hiding this comment.
| Now page scripts sees a new property on the window, `messenger`, which has a function `notify()`: | |
| Now page scripts will see a new property on the window, `messenger`, which has a function `notify()`: |
The original text is more correct. This is describing a desired effect, not a fact, so future tense is better.
There was a problem hiding this comment.
Thank you for your feedback. The best practice for technical writing is to use the present tense e.g. https://developers.google.com/style/tense. It generally makes the content clearer and more direct. For example, here we both say now and will, so does a page script see the new property now or at some undefined point in the future? However, thanks for commenting as it's highlighted that I didn't correctly make the change and I will fix that.
There was a problem hiding this comment.
Ah, TIL. Well, as long as the grammar is fixed I'll be happy with this ^^
Summary
Add the documentation suggested by @Rob--W in cloneInto example code fails: Error: Encountered unsupported value type writing stack-scoped structured clone
#15059 to offer an option for cloning promises.
Related issues
Fixes #15059
Metadata
This PR…