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

[SDK-3735] Create tree-shakable Crypto module #1419

Merged
merged 9 commits into from
Nov 7, 2023

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Aug 3, 2023

Note: This PR is based on top of #1417; please review that one first.

This encapsulates the crypto functionality in a tree-shakable module. See commit messages for more details.

BaseRest: 87.17 KiB
BaseRest + Rest: 87.17 KiB
BaseRest + Crypto: 90.70 KiB
BaseRest + generateRandomKey: 90.73 KiB
BaseRest + getDefaultCryptoParams: 90.73 KiB
BaseRest + decodeMessage: 87.53 KiB
BaseRest + decodeEncryptedMessage: 91.05 KiB
BaseRest + decodeMessages: 87.64 KiB
BaseRest + decodeEncryptedMessages: 91.16 KiB
BaseRealtime: 144.00 KiB
BaseRealtime + Rest: 152.65 KiB
BaseRealtime + Crypto: 147.53 KiB
BaseRealtime + generateRandomKey: 147.56 KiB
BaseRealtime + getDefaultCryptoParams: 147.56 KiB
BaseRealtime + decodeMessage: 144.36 KiB
BaseRealtime + decodeEncryptedMessage: 147.88 KiB
BaseRealtime + decodeMessages: 144.46 KiB
BaseRealtime + decodeEncryptedMessages: 147.99 KiB
generateRandomKey: 73.80 KiB
generateRandomKey + Crypto: 73.80 KiB
getDefaultCryptoParams: 73.80 KiB
getDefaultCryptoParams + Crypto: 73.80 KiB
decodeMessage: 70.61 KiB
decodeEncryptedMessage: 74.13 KiB
decodeEncryptedMessage + Crypto: 74.13 KiB
decodeMessages: 70.71 KiB
decodeEncryptedMessages: 74.23 KiB
decodeEncryptedMessages + Crypto: 74.23 KiB

Resolves #1396.

@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 3, 2023 12:57 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc August 3, 2023 12:58 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report August 3, 2023 12:58 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1396-Crypto-tree-shaking branch 2 times, most recently from 4db984d to ac1b598 Compare August 3, 2023 13:36
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 3, 2023 13:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report August 3, 2023 13:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc August 3, 2023 13:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 3, 2023 13:48 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc August 3, 2023 13:49 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report August 3, 2023 13:49 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 3, 2023 14:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report August 3, 2023 14:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc August 3, 2023 14:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 3, 2023 18:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc August 3, 2023 18:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report August 3, 2023 18:06 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch 2 times, most recently from 214c2cf to afb0235 Compare August 15, 2023 13:19
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 15, 2023 14:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report August 15, 2023 14:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc August 15, 2023 14:08 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch from afb0235 to 8091660 Compare August 15, 2023 16:45
@lawrence-forooghian lawrence-forooghian force-pushed the 1396-Crypto-tree-shaking branch 2 times, most recently from e45c239 to 6e6c3a1 Compare August 15, 2023 17:26
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features August 15, 2023 17:27 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc October 26, 2023 16:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report October 26, 2023 16:40 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch from 1be851b to 7e21e56 Compare October 26, 2023 17:21
@github-actions github-actions bot temporarily deployed to staging/pull/1419/features October 26, 2023 17:22 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/typedoc October 26, 2023 17:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1419/bundle-report October 26, 2023 17:23 Inactive
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

Looks good overall, one question though:

src/platform/web/index.ts Show resolved Hide resolved
@lawrence-forooghian lawrence-forooghian marked this pull request as draft November 1, 2023 18:32
@lawrence-forooghian lawrence-forooghian force-pushed the 1415-tree-shaking-attempt-2 branch 2 times, most recently from 70bee18 to abec79c Compare November 6, 2023 19:09
The type of this property in ably.d.ts does not permit it to be absent
or null. So, instead, throw an error if the user attempts to access it
when not set (for example, in the noencryption variant of the library).
Preparation for #1396 (making crypto functionality tree-shakable).
A bit clearer. Also use a named instead of default export.
We have decided that we don’t want the tree-shakable version of the
library to make use of static properties or methods, as they are not
amenable to tree-shaking. There’s also no obvious way to set a static
property in the API that we’ve chosen for tree-shaking (since we pass
dependencies into the class’s constructor).

Instead, the tree-shakable version of the library now exports standalone
functions which replace the removed static methods.
I want to use it in a second test file.
This removes the static BaseClient.Message property and the static
Message.{fromEncoded, fromEncodedArray} methods.

The motivation, and the approach taken for the tree-shakable and non
tree-shakable versions of the library, are as in c859b90.

Note that instead of fromEncoded and fromEncodedArray, the standalone
functions are named decodeMessage and decodeMessages — after some
discussion we decided that this naming was more consistent with the kind
of naming used for standalone functions in JavaScript libraries.
We move the crypto functionality into a tree-shakable Crypto module.

Then, we split the decodeMessage* functions introduced in 601b46b in
two; we introduce new decodeEncryptedMessage* variants which import the
Crypto module and are hence able to decrypt encrypted messages, and we
change the existing functions to not import the Crypto module and to
fail if they are given cipher options.

Resolves #1396.
Base automatically changed from 1415-tree-shaking-attempt-2 to integration/v2 November 6, 2023 19:47
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review November 7, 2023 11:58
@lawrence-forooghian lawrence-forooghian merged commit 8dbfcc7 into integration/v2 Nov 7, 2023
11 checks passed
@lawrence-forooghian lawrence-forooghian deleted the 1396-Crypto-tree-shaking branch November 7, 2023 11:58
@VeskeR VeskeR mentioned this pull request Mar 1, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants