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

Remove deprecated uses of window.postMessage #3649

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

esanzgar
Copy link
Contributor

@esanzgar esanzgar commented Aug 5, 2021

After #3611 there is no need to support window.postMessage on RPC and Bridge classes.

@esanzgar esanzgar linked an issue Aug 5, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #3649 (0816763) into master (67d3924) will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3649      +/-   ##
==========================================
+ Coverage   98.67%   98.89%   +0.21%     
==========================================
  Files         210      210              
  Lines        7716     7671      -45     
  Branches     1744     1731      -13     
==========================================
- Hits         7614     7586      -28     
+ Misses        102       85      -17     
Impacted Files Coverage Δ
src/shared/bridge.js 100.00% <100.00%> (+1.29%) ⬆️
src/shared/frame-rpc.js 100.00% <100.00%> (+20.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67d3924...0816763. Read the comment docs.

@esanzgar esanzgar requested a review from robertknight August 5, 2021 17:33
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM. Added a minor note about the RPC class documentation. It might be a good idea as a follow-up to rename the RPC and frame-rpc classes to reflect the fact that they now use MessagePort (PortRPC for the class name and port-rpc.js for the module name?).

@@ -59,42 +59,15 @@ const PROTOCOL = 'frame-rpc';
*/
export class RPC {
/**
* Create an RPC client for sending RPC requests from `sourceFrame` to
* `destFrame`, and receiving RPC responses from `destFrame` to `sourceFrame`.
* Create an RPC client for sending and receiving RPC message using a
Copy link
Member

Choose a reason for hiding this comment

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

The class documentation could use an update as well. eg.

RPC provides remote procedure calls between frames.

It uses the Channel Messaging API [1] for inter-frame communication.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Channel_Messaging_API

After #3611 there is no more need to support `window.postMessage` on `RPC` and `Bridge` classes.
@esanzgar esanzgar force-pushed the remove-deprecated-functionality branch from c63ee46 to 2e65209 Compare August 6, 2021 13:51
@esanzgar
Copy link
Contributor Author

esanzgar commented Aug 6, 2021

I updated the class documentation.

The `methods` argument constructor could be an object that uses `this`.
However, we do not rely in this feature, so I am suggesting to simplify
the execution of the callbacks.
@esanzgar esanzgar merged commit f5cc230 into master Aug 6, 2021
@esanzgar esanzgar deleted the remove-deprecated-functionality branch August 6, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove support for window.postMessage from RPC and Bridge
2 participants