Remove the 'ENVIRONMENT=worker implies ENVIRONMENT=web' assumption

[juj]

Remove the 'ENVIRONMENT=worker implies ENVIRONMENT=web' assumption. Fixes #25414 (review). Add test to verify the expectation mentioned in that comment.

[sbc100]

I suppose there is not much benefit in a program that only runs in webworker and therefore can elide any code related to the browser main thread?

My previous assumption is that that is what this test was testing.

[juj]

a program that only runs in webworker and therefore can elide any code related to the browser main thread?

Ohhh. hmmm that is an interesting idea. I suppose there could be such an optimization opportunity, although we would then have to have a worker-web vs worker-node distinction to separate between browser and Node Workers? (so passing -sENVIRONMENT=worker is still ambiguous whether it means web vs Node worker?)

[sbc100]

Maybe add a test for the new error message?

[kripken]

Please add a changelog entry for this breaking change.

[juj]

Hmm, actually taking a step back.. would such a -sENVIRONMENT=node-worker ever be a use case?

That is, would there ever be such a thing as "My output targets Node.js, but only inside a pthread/worker in Node"?

Because if not (and it does seem to me that would never be a thing?), then we would not need to disambiguate worker-web vs worker-node, but we could take the convention that -sENVIRONMENT=worker means exactly what Sam mentioned above: the code is only ever to run inside a web browser, and there only in a Worker, but never on the main browser thread?

Maybe that was the original intent of -sENVIRONMENT=worker? (I can't quite recall.. I've been familiar only with -sENVIRONMENT=web, -sENVIRONMENT=web,worker, and -sENVIRONMENT=node use in general)

[juj]

Actually yeah, that looks like what the original test intent of browser.test_pthreads_started_in_worker_limited_env was, now that I read it closer.

Adjusted the PR here to do a better fix for the problem. How does this look now?

[kripken]

This change makes sense to me, but it is a breaking change, so please mention it in the changelog.

[juj]

Hmm I don't think this should break any previous use case, but fix the previous regression? Which breakage are you referring to?

[kripken]

Will this not start to error on people passing ENVIRONMENT=worker and that were depending on that enabling "web" as well?

[juj]

Will this not start to error on people passing ENVIRONMENT=worker and that were depending on that enabling "web" as well?

The understanding here was that passing -sENVIRONMENT=worker has never intended to imply to enable web as well, but it was just my PR #25414 from last week that accidentally made it so (due to my misunderstanding of the situation), resulting in an accidental regression.

At least the test browser.test_pthreads_started_in_worker_limited_env seems to behave like -sENVIRONMENT=worker should only mean running in a web browser Worker, but not on the main thread.

[kripken]

Yeah, since it's just from last week maybe it's not worth a mention. But it did make it into a release, so it is noticeable, that's what worries me. But I don't feel strongly.

[juj]

But it did make it into a release

Oh I see.. yeah, I'll add a ChangeLog entry. How does that look?

[kripken]

Thanks, lgtm

[kripken]

Please add the version (4.0.16?)

[juj]

ok

[kripken]

Thanks!