extension: Don't build vanilla WASM module for web extensions #19316
Conversation
danielhjacobs
added
A-web
|
Note: I can't really test this properly, as I cannot "Release Nightly". I did test that |
|
Note: May increase minimum Chrome and Edge versions for the extension to version 96, released over 3 years ago: https://v8.dev/blog/v8-release-96 |
Does #19348 affect this then? 🤔 |
Someone will need to test the Safari extension with that PR. I don't have desktop Safari to test it. But that PR says it fixes the extension on Safari 16+. I don't know if the extension works on Safari 15- right now. With this PR, it will not. |
|
Okay but can we assume that 16.0, 16.1, and 16.2 are not in use anywhere anymore, because 16.3 is out? 🤔 |
|
16.4* At my last check, almost 5% of ios users are still using 16.0-16.3 for some reason, but unlike major versions, we can tell people to upgrade if they use those versions. Any device that supports 16.0 supports 16.4 too. People just don't upgrade unless they have to do so it seems. |
|
"Percent iOS 16.0-3" |
|
As of December, the market share is down to exactly 4%. |
This might be relevant here. |
| run: zip -r "${{ needs.create-nightly-release.outputs.package_prefix }}-web-selfhosted.zip" . | ||
| working-directory: web/packages/selfhosted/dist | ||
|
|
||
| - name: Upload selfhosted | ||
| if: ${{ !matrix.demo }} | ||
| if: matrix.demo |
There was a problem hiding this comment.
Why is the negation removed here? 🤔
There was a problem hiding this comment.
So selhosted continues working on Safari 16.3-. It's also why the workflow's long name is changed to "Build web demo and selfhosted"
There was a problem hiding this comment.
See the second paragraph of #19316 (comment)
There was a problem hiding this comment.
Then, since it now means "demo and selfhosted", maybe this flag should be replaced with a different flag with inverse values, for "extensions"? 🤔
There was a problem hiding this comment.
Oh but it should be clear that this flag will govern "whether we're building the browser extensions, or everything else", and not "whether WASM extensions are enabled".
There was a problem hiding this comment.
Hmm, looking at it again, more closely: Since (almost) all steps of the build-web job in the Release Nightly workflow are gated by the value of matrix.demo, maybe it would make more sense to just split it, getting rid of all the conditionals? 🤔
There was a problem hiding this comment.
Sure, a couple of setup steps at the beginning will have to be duplicated this way, but maybe the increased clarity is worth it...?
BTW still waiting for actions/runner#1182 or something similar...
There was a problem hiding this comment.
Something like this: 3299304 (#19767)
torokati44
force-pushed
the
no-dual-wasm-extensions
branch
from
41c76be to
4bb4d88
Compare
|
For posterity, copying my message from Discord:
Also note that I think I prefer the #19767 version of this PR. |
This won't support Safari 16.3- with the Safari extension, but the Safari extension doesn't work anyway, and by the time someone fixes that we can probably just drop Safari extension support for older Safari versions.
The selfhosted and npm packages are now being built as part of the "Build web demo" job. The "Build web demo" job still uses dual-wasm. It is now renamed the "Build web demo and selfhosted" job, and the "Build web" job is now the "Build web extensions" job (though it does also do things like cloning the JS docs still).