Skip to content

promise cloneinto clarification #19125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 12, 2022
Merged

promise cloneinto clarification #19125

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef68846
promise cloneinto clarification
rebloor Aug 3, 2022
9b975b3
corrections
rebloor Aug 4, 2022
b0a84b2
Created separate promise cloning section
rebloor Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
promise cloneinto clarification
  • Loading branch information
@rebloor
rebloor committed Aug 3, 2022
commit ef688469f69f6612a0ebdfb1a9e4aff31c994660
18 changes: 16 additions & 2 deletions .../en-us/mozilla/add-ons/webextensions/sharing_objects_with_page_scripts/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ In Firefox, part of the isolation between content scripts and page scripts is im

The purpose of this feature is to make it harder for the less-privileged script to confuse the more-privileged script by redefining the native properties of objects.

So for example, when a content script accesses the page's [window](/en-US/docs/Web/API/Window), it won't see any properties the page script added to the window, and if the page script has redefined any existing properties of the window, the content script will see the original version.
So, for example, when a content script accesses the page's [window](/en-US/docs/Web/API/Window), it won't see any properties the page script added to the window, and if the page script has redefined any existing properties of the window, the content script will see the original version.

## Accessing page script objects from content scripts

Expand Down Expand Up @@ -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()`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, TIL. Well, as long as the grammar is fixed I'll be happy with this ^^


```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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this after the next section and add a header such as "Promise cloning"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rob--W to clarify, you want it moved after the "Constructors from the page context" section?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, move it after that, and prepend a header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


```js
let promise = new window.Promise(resolve => {
// if just a primitive, then cloneInto is not needed:
// resolve("string is a primitive");

// if not a primitive, such as an object, then the value must be cloned
let result = { exampleKey: "exampleValue" };
resolve(cloneInto(result, window));
});
// now promise can be passed to the web page.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpl Is this sufficiently clear, or do we need to elaborate on how the value is passed to the webpage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sgtm

```

### Constructors from the page context

On the xrayed window object pristine constructors for some built-in JavaScript objects such as `Object`, `Function` or `Proxy` and various DOM classes are available. `XMLHttpRequest` does not behave in this way, see the [XHR and fetch](/en-US/docs/Mozilla/Add-ons/WebExtensions/Content_scripts#xhr_and_fetch) section for details. They will create instances belonging to the page global's object hierarchy and then return an xray wrapper.
Expand Down