Import base64 utils directly from js-sdk #12871
Conversation
See comments in code
turt2live
added
the
T-Task
|
This appears to fix element-hq/element-web#27759, presumably because Webpack changes strategy with a smaller file. |
|
sigh
|
| // is used by Element Web's service worker, and importing `matrix` brings in ~1mb of stuff | ||
| // we don't need. Instead, we ignore the import restriction and only bring in what we actually | ||
| // need. | ||
| // eslint-disable-next-line no-restricted-imports |
There was a problem hiding this comment.
Note that this means that react-sdk builds can be broken by minor releases of the js-sdk. matrix-js-sdk/src/base64 is not a public interface.
There was a problem hiding this comment.
encodeUnpaddedBase64 is approximately one line of code, if you don't need to maintain compatibility with node.js. Maybe just redefine it, rather than going all left-pad on it?
There was a problem hiding this comment.
how do we make it a public interface?
There was a problem hiding this comment.
ugh, github only showing half the comments is unhelpful :|
I'm a little worried about it breaking in some weird way, but presumably it's been stable for 10 years now, so...
There was a problem hiding this comment.
how do we make it a public interface?
js-sdk's public interfaces are defined in its README, https://github.com/matrix-org/matrix-js-sdk?tab=readme-ov-file#entry-points, and in the documentation (http://matrix-org.github.io/matrix-js-sdk/index.html, left sidebar), so in theory, I guess you make a PR to add it.
Personally though, I'm a bit unconvinced this should live as a separate entrypoint.
I'm a little worried about it breaking in some weird way, but presumably it's been stable for 10 years now, so...
base64.js has only existed for a few months, iirc, so no I don't think it's been stable for 10 years.
There was a problem hiding this comment.
hmm, having tried to inline: the library claims to export an ArrayBuffer (a node thing), using web APIs, which has an incompatible interface with Uint8Array. The existing js-sdk code does this type checking for us already - it'd be a shame to copy/paste it too.
So, is there a way for us to declare base64 as public interface? If we don't think it belong as js-sdk, maybe we create our own package for it. I'm somewhat biased towards short term solutions though, as I'd like to get this into an RC cycle.
There was a problem hiding this comment.
(ffs github, show me comments when I reload and not after I post)
I'm going to go the route of README changes then.
There was a problem hiding this comment.
... though yea, it's a bit silly as its own entrypoint. Maybe we just accept and document the risk?
There was a problem hiding this comment.
I get that we should prefer the public interface in the general case, but personally I would think this is an acceptable risk - matrix-react-sdk is continuously deployed with the latest git revision of matrix-js-sdk, and we also have CI that triggers upstream test runs for matrix-react-sdk, so any breaks are caught very early. We also lock the dependency to a specific version when releasing.
… so)" This reverts commit 835806d.
|
I did not expect that commit to work, but I won't decline a checkmark from the CI. @robintown please take a look at the test stuff and comment quality in particular - I'm not confident it helpfully describes what is actually going on here. |
Co-authored-by: Robin <[email protected]>
sw.jsin a production build was previously 1mb almost exactly. With this change, it's 37kb.