Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Import base64 utils directly from js-sdk #12871

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Import base64 utils directly from js-sdk #12871

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6356113
Import base64 utils directly from js-sdk
turt2live Aug 6, 2024
835806d
Use the authenticated routes (because the service worker said so)
turt2live Aug 6, 2024
8003cf8
Revert "Use the authenticated routes (because the service worker said…
turt2live Aug 6, 2024
400d7fb
Use the authenticated routes (because the service worker said so)
turt2live Aug 6, 2024
d05417a
Continue fighting Playwright
turt2live Aug 6, 2024
a2a1ae1
Document who is at fault if the import breaks (it's us)
turt2live Aug 6, 2024
032ed46
Update playwright/e2e/timeline/timeline.spec.ts
turt2live Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/utils/tokens/pickling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { encodeUnpaddedBase64 } from "matrix-js-sdk/src/matrix";
// Note: we don't import the base64 utils from `matrix-js-sdk/src/matrix` because this file
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we make it a public interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ffs github, show me comments when I reload and not after I post)

I'm going to go the route of README changes then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... though yea, it's a bit silly as its own entrypoint. Maybe we just accept and document the risk?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import { encodeUnpaddedBase64 } from "matrix-js-sdk/src/base64";
import { logger } from "matrix-js-sdk/src/logger";

/**
Expand Down
Loading