Skip to content
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.

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

Use SetIntegrityLevel instead of Object.freeze() #125

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

annevk
Copy link
Member

@annevk annevk commented Feb 27, 2018

Verbose, but clear and correct.

Fixes #74.


Preview | Diff

Verbose, but clear and correct.

Fixes #74.
@annevk annevk requested review from johnmellor and domenic February 27, 2018 15:05
Copy link
Member

@beverloo beverloo left a comment

Choose a reason for hiding this comment

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

LGTM from a functional point of view. (John no longer works on Chrome.)

<li><p>Append <var>action</var> to <var>frozenActions</var>.
<li><p>Perform <a abstract-op>SetIntegrityLevel</a>(<var>jsAction</var>, "frozen").

<li><p>Let <var>idlAction</var> be <var>jsAction</var>, <a>converted to an IDL value</a>.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't correct, at least as written. The conversion, presumably to an IDL NotificationAction type (although that isn't stated), will re-perform the Get()s of all the properties, which can be observable if Object.prototype is modified. Then we'll convert back to JS in the binding layer.

I don't think there's any way to do this rigorously as long as your return type is FrozenArray<NotificationAction> (instead of FrozenArray<object>). The conversion between JS objects and dictionaries is observable and side effecting.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot use FrozenArray then either since it's constructor takes an IDL value. I'm not sure I want to inline all of that. That would be rather bad and I'd likely make mistakes or it would result in mismatches down the line.

There is some mismatch here though with implementations I think where JS values are backed by IDL values and you can manipulate either. That's not conversion, but just grabbing the corresponding value, but IDL doesn't formalize that.

Base automatically changed from master to main January 15, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

"Call Object.freeze on action"
3 participants