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

ethers is not serviceWorker-compatible #506

Closed
cellog opened this issue May 6, 2019 · 11 comments
Closed

ethers is not serviceWorker-compatible #506

cellog opened this issue May 6, 2019 · 11 comments
Labels
enhancement New feature or improvement.

Comments

@cellog
Copy link

cellog commented May 6, 2019

Because the JsonRpcProvider uses fetchJson, which uses XMLHttpRequest, ethers doesn't work in a service worker.

Would you be amenable to replacing fetchJson with just fetch?

If not, can we discuss ways to make it easy to replace (like adding a fetchJson method to JsonRpcProvider that can be overridden to call fetch instead of hard-coding calls to fetchJson?)

@ricmoo
Copy link
Member

ricmoo commented May 6, 2019

This has come up before, and I'm slightly leaning towards fetch, the only problem is that it is not available in certain environments. I need to do another analysis of fetch vs. XmlHttpRequest.

This change cannot be added to v4, as that would break existing users. But I'm about to release v5, so I will look into this change for that; v5 will spend some time in beta, so we can try changing it throughout the beta to see how it affects people.

The test cases run in phantomjs, which I do no believe has fetch. And I need to look at the recent versions of Duktape. Maybe instead of a shim for XmlHttpRequest that uses fetch, I can shim fetch with one that uses XmlHttpRequest.

For now, there are XmlHttpRequest polyfils you can use (which is what the node version actually uses). I should also add it to the existing shims too.

It is actually quite trivial to sub-class the JsonRpcProvider and just override send, but that only gets you JsonRpcProvider, a nicer low-level solution would also get Etherscan et al. for free. :)

@ricmoo ricmoo added discussion Questions, feedback and general information. enhancement New feature or improvement. next version on-deck This Enhancement or Bug is currently being worked on. labels May 6, 2019
@cellog
Copy link
Author

cellog commented May 7, 2019

this all sounds great, and yes we will sub-class JsonRpcProvider for our purposes. A stopgap for v4 is actually quite simple: in fetchJson, check to see if fetch is defined on self and if so, use it, otherwise fall back.

What do you think? The API is the same as fetchJson

@cellog
Copy link
Author

cellog commented May 7, 2019

...I take it back. The API is slightly different. I'm happy to provide the code I am using if it would help

@ricmoo
Copy link
Member

ricmoo commented May 8, 2019

Actually, thinking about this more. Are you sure XmlHttpRequest is not working for you? Could it just be a CORS issues, which I think Web Workers need a little finessing for.

Can you try setting the connection object, instead of just the URL?

// Instead of this:
let url = "http:/..."
//let provider = new JsonRpcProvider(url);

// Use this:
let connection = {
    headers: { "X-Requested-With": "XMLHttpRequest", "Access-Control-Allow-Origin": "*"}
    url: url
};
let provider = new JsonRpcProvider(connection);

You may need to adjust the CORS headers for your situation, but I think this might solve the issue...

@cellog
Copy link
Author

cellog commented May 8, 2019

yes I am sure, XmlHttpRequest does not exist in the worker context. https://stackoverflow.com/questions/37112425/xmlhttprequest-within-service-worker

Thanks for taking the time to think about it!

@ricmoo
Copy link
Member

ricmoo commented Jun 12, 2019

The v5 branch now uses fetch instead of XMLHttpRequest, if you’d like to try it out. :)

@cellog
Copy link
Author

cellog commented Jun 12, 2019

Awesome work! Thanks Richard

@ricmoo ricmoo removed discussion Questions, feedback and general information. on-deck This Enhancement or Bug is currently being worked on. labels Jun 12, 2019
@ricmoo
Copy link
Member

ricmoo commented Jul 15, 2019

I'm going to close this now. This change will not be ported back to v4, since it could introduce non-backwards-compatible issues, and would also require re-writing all the test cases, which is out of scope for v4 as we are quite close to v5 being moved out of beta.

Feel free to re-open though to continue discussion.

Thanks! :)

@ricmoo ricmoo closed this as completed Jul 15, 2019
@kamescg
Copy link

kamescg commented Apr 18, 2020

Just wondering if v5 reverted back to XMLHttpRequest?

Ran the build locally, and loaded the full UMD build into a service worker. It seems there is a single XMLHttpRequest in the build that causes an error to throw when initializing a new provider and running provider.on('block', ...).

@ricmoo
Copy link
Member

ricmoo commented Apr 18, 2020

Ugh... I just checked and yes. The WebSocket library pulls it in, and it looks like it might be doing some weird polyfil stuff, hijacking the actual fetch.

I'll look into this and figure out how to suppress it.

@ricmoo ricmoo reopened this Apr 18, 2020
@ricmoo ricmoo closed this as completed Apr 18, 2020
@ricmoo
Copy link
Member

ricmoo commented Apr 18, 2020

Actually, rather than re-opening, I'm going to open. new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement.
Projects
None yet
Development

No branches or pull requests

3 participants