-
Notifications
You must be signed in to change notification settings - Fork 6
feat(extension): Add distributed object capability programming #43
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
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
d8c48c4 to
3677520
Compare
|
@SocketSecurity ignore npm/@endo/captp@4.3.0 npm/@endo/exo@1.5.3 npm/@endo/patterns@1.4.3 We have a close working relationship with the @Endo contributors and can rely on them for defensively correct implementations. |
|
@SocketSecurity ignore npm/rimraf@3.0.2 We explicitly require rimraf^6.0.1 |
3f638e3 to
918f800
Compare
This comment was marked as resolved.
This comment was marked as resolved.
822f49f to
4c5788a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
…-less implementations.
4c5788a to
19f16cb
Compare
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! See inline comments.
packages/extension/src/message.ts
Outdated
| | DataObject[] | ||
| | { [key: string]: DataObject }; | ||
|
|
||
| export enum ExtensionTarget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to put this in an enum, but might I suggest MessageTarget instead for the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about ExtensionMessageTarget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd work, although we're in the extension package, and there is only one kind of message. Also, if we do ExtensionMessageTarget, we should rename MessageId to ExtensionMessageId. Do you see a need to disambiguate it from anything in particular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do.
The word Extension in this enum is to clarify that there are a finitely enumerable set of targets to which one can send an ExtensionMessage, and those targets are enumerated by ExtensionMessageTargets. The underlying extension framework responds to the targets 'background' and 'offscreen', so these are extension targets.
MessageId is only used when sending messages to iframes, or in this implementation the iframe. As we approach target key. That may be a faulty reservation. But in any case we don't use MessageId when sending an ExtensionMessage, so we shouldn't rename it to ExtensionMessageId.
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
Refs: #23
Status