content script structured clonable errors should be warnings

REOPENED
Unassigned

Status

()

Toolkit
WebExtensions: General
REOPENED
15 days ago
8 days ago

People

(Reporter: dietrich, Unassigned)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 wontfix)

Details

(Reporter)

Description

15 days ago
Currently if a content script doesn't return a structured clonable, an error is thrown.

Error: Script 'moz-extension://9374effe-733e-ba44-b9ca-600b1aa42003/content-script.js' result is non-structured-clonable data

However, scripts do not have to return anything at all. All my use-cases are scripts that don't return anything to the extension code.

This should be a warning, at most. Possibly shouldn't do anything at all.
I assume we're talking about browser.tabs.executeScript() here?
Your comment doesn't make much sense, you don't care about return values in some particular case but executeScript() is documented to resolve with the script's return value.  If it is unable to implement its documented behavior, it throws an Error, that is entirely appropriate.
Having the caller specify whether they care about return values would also simplify this but that would not be compatible with other browsers that implement this API.
If you don't care about return values, you can just add `undefined;` at the end of your script.

Updated

14 days ago
status-firefox57: --- → wontfix
(Reporter)

Comment 2

14 days ago
(In reply to Andrew Swan [:aswan] from comment #1)
> I assume we're talking about browser.tabs.executeScript() here?

Yep, sorry for not including that very important piece of information!

> If you don't care about return values, you can just add `undefined;` at the
> end of your script.

Yes, it's quite obvious that is possible.

This bug is about the fact that the API requires the developer to jump through that hoop in the first place.

Not returning a value is not an error state.

This is what we would call a "developer papercut" bug. It's not lethal, but it's also entirely unnecessary.
(In reply to Dietrich Ayala (:dietrich) from comment #2)
> This bug is about the fact that the API requires the developer to jump
> through that hoop in the first place.
> 
> Not returning a value is not an error state.
> 
> This is what we would call a "developer papercut" bug. It's not lethal, but
> it's also entirely unnecessary.

There's a big difference between something being annoying and it being "entirely unnecessary".  And this one is the former.  If we swallow this error, we will break users of executeScript() that do care about the return value.  Like I said before, the real fix would be to have callers explicitly indicate that they want the return value but we can't do that without breaking cross-browser compatibility.
How is not returning a value different from returning undefined?
(In reply to Tom Schuster [:evilpie] from comment #4)
> How is not returning a value different from returning undefined?

This is about a script's completion value, not the return value from a function call.  This is more your area of expertise, but I don't know a way of forcing a script completion value to undefined without explicitly writing "undefined;"
Resolving per earlier comments
Status: NEW → RESOLVED
Last Resolved: 8 days ago
Resolution: --- → INVALID
(Reporter)

Comment 7

8 days ago
Andrew, premature bug closure is a *really* poor way of treating contributors.

We should be encouraging discussion and exploring creative solutions to these types of problems, not just shutting down bugs.

(In reply to Andrew Swan [:aswan] from comment #3)
> There's a big difference between something being annoying and it being
> "entirely unnecessary".  And this one is the former.  If we swallow this
> error, we will break users of executeScript() that do care about the return
> value.  Like I said before, the real fix would be to have callers explicitly
> indicate that they want the return value but we can't do that without
> breaking cross-browser compatibility.

Restating from above:

*Not returning a value is not an error state.*

We should *not* be treating it as one.

I suggested an easy middle-ground solution above: Let's make it a warning.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
You need to log in before you can comment on or make changes to this bug.