🚧 Add WebChannel RPC transport layer. #181
Conversation
Chralu
force-pushed
the
webchannel_rpc
branch
2 times, most recently
from
2c2db3e to
d4caac7
Compare
| this._state = AWCStreamChannelState.CONNECTING | ||
|
|
||
| console.log(`[AWC] Wait for popup extension ...`) | ||
| await chrome.runtime.sendMessage(this.extensionId, 'ensureExtensionPopupOpened') |
There was a problem hiding this comment.
I saw on MDN doc it's prefered to support cross platform api like suggest by:
Firefox also supports the chrome.* namespace for APIs that are compatible with Chrome, primarily to assist with porting. However, using the browser.* namespace is preferred. In addition to being the proposed standard, browser.* uses promises—a modern and convenient mechanism for handling asynchronous events.
Maybe using a polyfill or using the browser.* can leverage firefox & chrome at the same time
There was a problem hiding this comment.
browser.* is not supported by chrome yet.
Using a polyfill like this one seems like a requirement to get a wide compatibility.
I'll look at that in a second step 👍
src/endpoint.ts
Outdated
|
|
||
| if (url.protocol === "ws:") { | ||
| return new WalletRPCEndpoint(endpoint); | ||
| static build(endpoint: string | undefined): Endpoint { |
There was a problem hiding this comment.
What do you think to support String | Endpoint | undefined ? This way custom AWCEndpoint such as AWCWebBrowserExtension or any AWCEndpoint might be passed as argument
There was a problem hiding this comment.
In my understanding, that method is a factory. Its role is to choose which kind of Endpoint to instanciate for a given endpoint uri.
I guess you suggest that change because once AWCWebBrowserExtension will be extracted from libjs, build method won't be able to build the AWCEndpoint anymore.
Maybe the solution would be to follow your next proposal 👇
| /** | ||
| * @param endpoint if undefined, endpoint will be resolved using ArchethicWalletClient. | ||
| */ | ||
| constructor(endpoint: string | undefined) { |
There was a problem hiding this comment.
Probably the same remark/question as above
There was a problem hiding this comment.
I Agree with you.
We could directly pass an Endpoint instance here. This will be mandatory to extract AWCWebBrowserExtention from here.
There was a problem hiding this comment.
After more thought, this would be a breaking change.
Looking at the number of PRs waiting for validation, I think that change should wait a bit.
No description provided.