Add url data encoding #345
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Relates to #327 |
felicio
commented
Feb 21, 2023
Comment on lines
14
to
34
| test('A', () => { | ||
| const data = { | ||
| displayName: 'Lorem ipsum dolor sit egestas.', | ||
| description: | ||
| 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus non dui vitae augue elementum laoreet ac pharetra odio. Morbi vestibulum.', | ||
| membersCount: 1_000_000, | ||
| color: '#4360DF', | ||
| } | ||
| const encodedData = encodeUrlData('community', data, { | ||
| serialization: 'csv', | ||
| compression: 'noop', | ||
| encoding: 'encodeURIComponent', | ||
| }) | ||
| const characterLength = countCharacters(data) | ||
|
|
||
| expect(characterLength).toBe(184) | ||
| expect(encodedData).toBe( | ||
| 'Lorem%20ipsum%20dolor%20sit%20egestas.%2CLorem%20ipsum%20dolor%20sit%20amet%2C%20consectetur%20adipiscing%20elit.%20Phasellus%20non%20dui%20vitae%20augue%20elementum%20laoreet%20ac%20pharetra%20odio.%20Morbi%20vestibulum.%2C1000000%2C%234360DF' | ||
| ) | ||
| expect(encodedData).toHaveLength(243) | ||
| }) |
There was a problem hiding this comment.
I contemplated leaving these comparison test cases in the repo, but deciding against it.
felicio
force-pushed
the
f/decode-url-data
branch
from
d0a6eeb
to
f26ef66
Compare
felicio
commented
Feb 21, 2023
| }).toBinary() | ||
| const compressed = brotliCompressSync(serialized) | ||
| // todo?!: remove padding | ||
| // todo?!: split into 301 chars chunks with a semicolon separator |
There was a problem hiding this comment.
Haven't reached 301 threshold, at which iOS/macOS splits up links, for sample data containing names and descriptions consisting of just a limited ASCII set which product intends to only support, but approaching. See
On the other hand, considering other languages (Unicode) the threshold is exceeded. See
- https://github.com/felicio/status-web/blob/825262c4f07a68501478116c7382862607a5544e/packages/status-js/src/utils/encode-url-data.compare.test.ts#L430 (
360) - and https://github.com/felicio/status-web/blob/825262c4f07a68501478116c7382862607a5544e/packages/status-js/src/utils/encode-url-data.compare.test.ts#L457 (
432)
Finally, without brotli results are longer. See
- https://github.com/felicio/status-web/blob/825262c4f07a68501478116c7382862607a5544e/packages/status-js/src/utils/encode-url-data.compare.test.ts#L79 (
252) - vs. https://github.com/felicio/status-web/blob/825262c4f07a68501478116c7382862607a5544e/packages/status-js/src/utils/encode-url-data.compare.test.ts#L102 (
192)
felicio
commented
Feb 21, 2023
| checksum: getChecksum(data, publicKey), | ||
| }).toBinary() | ||
| const compressed = brotliCompressSync(serialized) | ||
| // todo?!: remove padding |
There was a problem hiding this comment.
It's the == that can be at the end of encoded results, and actual savings being max 2 chars.
felicio
commented
Feb 21, 2023
felicio
commented
Feb 21, 2023
prichodko
reviewed
Feb 23, 2023
prichodko
reviewed
Feb 23, 2023
prichodko
reviewed
Feb 23, 2023
|
Great PR otherwise. Very understandable. |
@prichodko like |
|
I was rather thinking something like |
|
I'm failing to see the benefit in splitting 😅. In short term, I see it only ever being used where |
felicio
commented
Mar 14, 2023
felicio
force-pushed
the
f/decode-url-data
branch
from
162d146
to
dbd99ec
Compare
felicio
added a commit
to felicio/status-web
that referenced
this pull request
May 15, 2023
* add @scure/base * add link-preview.proto * add prototype of encode function * add tests * add test cases * update proto * more * more * add missing community chat description to proto * more * more * add browser brotli and lz-string * move encoding comparison * add sinlge encoding * split encoding * add decoding * update .prettierignore * exclude comparison * remove comparison tests * Update packages/status-js/src/utils/encode-url-data.test.ts * Update packages/status-js/src/utils/encode-url-data.test.ts * remove checksum * ensure channel is serializable * Update .prettierignore * update protos * add creaet-url.ts * set links * comment * update protos * add nominal type for EncodedUrlData * add sign/verify fns * export fns from index * set zlib as external module * add tag indices * encode channel uuid * use `.toJson()` with type assertion * use uppercase url * split url creating fns * fix typo * describe test suite * use getters * fix nominal type * remove `node:` prefix from `zlib` import * remove todos?: * rename URLProps to URLParams * fix package.json after rebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements status-im/specs#169