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 noProxy option from -sPROXY_TO_WORKER #23297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 4, 2025

The noProxy options was added back in 2014 in c2295c7, but doesn't have any test coverage. This means there is is good chance its already broken. What is more it a fair mount of complexity since it adds an additional way in which certain JS files get pre-processed and included.

I propose that we simply remove it, but and alternative would be to fine a way to test it. I'm not quite sure who the end user of this feature might be though.

The `noProxy` options was added back in 2014 in c2295c7, but doesn't
have any test coverage.  This means there is is good chance its already
broken.  What is more it a fair mount of complexity since it adds an
additional way in which certain JS files get pre-processed and included.

I propose that we simply remove it, but and alternative would be to fine
a way to test it.  I'm not quite sure who the end user of this feature
might be though.
@sbc100 sbc100 changed the title Remove noProxy options from -sPROXY_TO_WORKER Remove noProxy option from -sPROXY_TO_WORKER Jan 4, 2025
@sbc100 sbc100 requested a review from kripken January 4, 2025 06:26
@kripken
Copy link
Member

kripken commented Jan 6, 2025

doesn't have any test coverage

Was it not tested here?

c2295c7#diff-644556461d8f2f2bdd70b1f0d5455342db92e9535b5bca4ca5223224bd1a21f3R751

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 6, 2025

doesn't have any test coverage

Was it not tested here?

c2295c7#diff-644556461d8f2f2bdd70b1f0d5455342db92e9535b5bca4ca5223224bd1a21f3R751

Ah, you are correct. I was kind of hoping it wasn't tested and that it had been broken for a while :)

Do you want to keep it around?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 6, 2025

But how did the browser tests pass then ... ?

@kripken
Copy link
Member

kripken commented Jan 6, 2025

This option is probably not that important, given it is proxy-to-worker and proxy-to-pthread is much more useful. And IIRC one can just build without the option to run on the main thread, which is all the option did. So I think I'm ok with removing it. Maybe worth asking on the mailing list first?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 7, 2025

Oh I see the test was removed in #13244. I can't really see why .. maybe because it was marked as @no_wasm_backend .. but still seems odd that I just removed it like that.

@kripken
Copy link
Member

kripken commented Jan 7, 2025

Hopefully we can remove it. lgtm if no one complains on the mailing list.

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.

2 participants