Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature: Native Emscripten support #3195
Feature: Native Emscripten support #3195
Changes from all commits
89bdcd6
2a77ea8
e6e9bb4
0cc345c
ec011f2
0819efe
4810c8b
8e2164e
667d54f
a0a5709
5dd3c4a
034ad0e
51364a4
b45cf16
1a00b78
09feff0
30d8b69
a45b32f
d8fb8ad
3c4bb66
2f89bae
7862c11
05c97dd
363a0f3
f871d98
e0d973b
5aaaab6
5e4b9ae
319d067
7918f0c
611e16d
242af7b
2771537
38be9df
a5ead7f
9afcec1
8978367
4321c9d
b5aa6e5
e34930c
35994ad
f62e0a4
62ad859
30b3063
ea67316
64d4ab6
bc8c096
9de656b
76d39b5
68ff2d0
1af9fdd
19964c2
985d8ca
0984732
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Have we considered not doing this by default?
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.
Yeah that seems correct, we can document that you need to call this for Emscripten support.
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.
Personally I really think it makes more sense to make it work out of the box on emscripten. Otherwise you end up with the situation that someone downloads a library which depends on requests or urllib3 and it inexplicably just doesn't work at all, and the only way they can make it work is to know that they need to read the dependency list, find urllib3 on it, and then read the urllib3 documentation to find that while urllib3 does support emscripten, you need to call a special function before urllib3 works on emscripten, and without it you just get a range of obscure not implemented errors inside socket We're already seeing similar with the current pyodide-http monkeypatch module, where you end up putting in code downstream for emscripten support that basically just looks for sys.platform and calls pyodide-http to monkeypatch everything.
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.
One possible thing to do would be to add a check for whether sockets are implemented into this code just in case something changes in the range of environments targeted by emscripten. Realistically though, sockets support is really unlikely in browser webasembly, so it is likely we'll remain in the situation where only http based protocols are available for at least the foreseeable future.
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 don't have a strong opinion, just thought it should be considered.
What are the arguments in favor of opting in? Explicit is better than implicit, mostly. Having urllib3 work magically could give the impression that sockets are supported and set unrealistic expectations. More hypothetically, having an explicit API gives us more options.
Your arguments against opting in do make sense though!
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.
We can stick to this for now, I want to poke around for a bit once this is merged too.