These utilities will be used to encode ThreadPermissions as minimally as possible.
Just starting with some utility functions and unit tests.
Details
Details
Naive unit tests pass, will add more.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/permissions/minimally-encoded-thread-permissions.js | ||
|---|---|---|
| 10–31 ↗ | (On Diff #32235) | It is really easy to make a mistake because it requires a lot of attention to distinguish between e.g. 0b1000000000000000000 and 0b10000000000000000000. How about using left shifts instead? |
| 37 ↗ | (On Diff #32235) | Do we have a way of statically checking if all the permissions have entries in minimallyEncodedThreadPermissions? E.g. can we assign a type to it? |
| lib/permissions/minimally-encoded-thread-permissions.js | ||
|---|---|---|
| 9 ↗ | (On Diff #32235) | It could be good to explain how to add a new permission here if someone was to add a new permission to the regular threadPermissions enum (or even a comment above that enum pointing them to add it here too). Also, it could be good to prevent people from accidentally excluding a permission from one of the enums (either threadPermissions or minimallyEncodedThreadPermissions) by adding a unit test that expects Object.keys(threadPermissions).length === Object.keys(minimallyEncodedThreadPermissions).length so we never run into a case where a new permission is introduced in one and not the other without it failing |
| 10–31 ↗ | (On Diff #32235) | Agreed |
| lib/permissions/minimally-encoded-thread-permissions.js | ||
|---|---|---|
| 11 ↗ | (On Diff #32235) | I wonder if we can skip this one. Won't the devices that use the new bitmask permissions already have this permissions wiped from their store |
Comment Actions
So BigInt works on keyserver/web/native (there was a known issue with pow on native, but we're not using that), but flow support only shows up in 0.194.0.
I did a super quick flow-bin upgrade just to see how many "breaking" flow issues got introduced, and quickly dropped the idea of upgrading flow-bin to support BigInt here.
There was no great solution, so I put in some annotations to suppress flow issues for now and went with some runtime checks. Please let me know if there are any better approaches here.
| .eslintrc.json | ||
|---|---|---|
| 5 ↗ | (On Diff #32346) | This is to get around eslint issues about BigInt not being defined. |
| lib/permissions/minimally-encoded-thread-permissions.js | ||
| 11 ↗ | (On Diff #32235) | We could, but I wanted to keep 1:1 parity w/ thread-permission-types: threadPermissions |
Comment Actions
Can you figure out which version of React Native brings in that version of Flow? If you need pointers on that let me know. We'll need a Linear task either way, but hopefully we can make it a follow-up for the current upgrade task
| .eslintrc.json | ||
|---|---|---|
| 5 ↗ | (On Diff #32346) | Can you give us more context on what env is for and what es2020 means here? I'm current eg. why we don't use a newer one. Is it supposed to represent the set of features that our JS tools (Node, Hermes, Jest, etc.) are able to use? |
| lib/permissions/minimally-encoded-thread-permissions.js | ||
| 15 ↗ | (On Diff #32346) | Let's remove membership |
| .eslintrc.json | ||
|---|---|---|
| 5 ↗ | (On Diff #32346) | s/current/curious |
Comment Actions
Can you figure out which version of React Native brings in that version of Flow? If you need pointers on that let me know. We'll need a Linear task either way, but hopefully we can make it a follow-up for the current upgrade task
Version of flow that introduces BigInt (WITH all of the necessary operations) is 0.194.0
RN 0.71-stable is on 0.191.0
RN 0.72-stable is on 0.202.0
So we'll need at least RN 0.72.x
| .eslintrc.json | ||
|---|---|---|
| 5 ↗ | (On Diff #32346) | Yeah it's supposed to represent what features our JS tools can use. Went with es2020 since that's the version of the EcmaScript standard that introduces full BigInt support across the board AND I'm confident that we support BigInt across tooling/platforms/etc because I did some testing. es2020 gives us what we need at the moment, and I didn't want to spend time investigating what the true "lowest common denominator" ES standard our tooling supports. Here's a rough overview of ES2021 ->  |
Comment Actions
Here's a Linear issue: https://linear.app/comm/issue/ENG-5431/revert-bigint-related-fixmes
And will go ahead and remove members in an update to this diff momentarily.
Comment Actions
Accepting, but please remove the membership permission before landing!
| .eslintrc.json | ||
|---|---|---|
| 5 ↗ | (On Diff #32346) | Thanks for explaining |
Comment Actions
Actually, running into the following when I remove that field
✖ yarn workspace landing flow --quiet:
error Command failed with exit code 2.
error Command failed.
Exit code: 2
Command: /nix/store/4v3g4sxwrfxpy24kfkgcv33dyh6r9lf3-nodejs-18.17.1/bin/node
Arguments: /nix/store/kxli919jahplrnfxc6q35wpgyfdy00ym-yarn-1.22.19/libexec/yarn/lib/cli.js flow --quiet
Directory: /Users/atul/comm/landing
Output:
$ /Users/atul/comm/node_modules/.bin/flow --quiet
Error ------------------------------------------------- ../lib/permissions/minimally-encoded-thread-permissions.js:49:52
Cannot get `minimallyEncodedThreadPermissions[key]` because property `membership` is missing in frozen object
literal [1]. [prop-missing]
../lib/permissions/minimally-encoded-thread-permissions.js:49:52
49| bitmask |= minimallyEncodedThreadPermissions[key];
^^^
References:
../lib/permissions/minimally-encoded-thread-permissions.js:11:57
v
11| const minimallyEncodedThreadPermissions = Object.freeze({
12| // TODO (atul): Update flow to `194.0.0` for bigint support
13| // $FlowIssue bigint-unsupported
14| know_of: BigInt(1) << BigInt(0),
15| visible: BigInt(1) << BigInt(1),
16| voiced: BigInt(1) << BigInt(2),
17| edit_entries: BigInt(1) << BigInt(3),
18| edit_thread: BigInt(1) << BigInt(4), // EDIT_THREAD_NAME
19| edit_thread_description: BigInt(1) << BigInt(5),
20| edit_thread_color: BigInt(1) << BigInt(6),
21| delete_thread: BigInt(1) << BigInt(7),
22| create_subthreads: BigInt(1) << BigInt(8), // CREATE_SUBCHANNELS
23| create_sidebars: BigInt(1) << BigInt(9),
24| join_thread: BigInt(1) << BigInt(10),
25| edit_permissions: BigInt(1) << BigInt(11),
26| add_members: BigInt(1) << BigInt(12),
27| remove_members: BigInt(1) << BigInt(13),
28| change_role: BigInt(1) << BigInt(14),
29| leave_thread: BigInt(1) << BigInt(15),
30| react_to_message: BigInt(1) << BigInt(16),
31| edit_message: BigInt(1) << BigInt(17),
32| edit_thread_avatar: BigInt(1) << BigInt(18),
33| manage_pins: BigInt(1) << BigInt(19),
34| manage_invite_links: BigInt(1) << BigInt(20),
35| });
^ [1]
Error ------------------------------------------------- ../lib/permissions/minimally-encoded-thread-permissions.js:64:63
Cannot get `minimallyEncodedThreadPermissions[permission]` because property `membership` is missing in frozen object
literal [1]. [prop-missing]
../lib/permissions/minimally-encoded-thread-permissions.js:64:63
64| const permissionBitmask = minimallyEncodedThreadPermissions[permission];
^^^^^^^^^^
References:
../lib/permissions/minimally-encoded-thread-permissions.js:11:57
v
11| const minimallyEncodedThreadPermissions = Object.freeze({
12| // TODO (atul): Update flow to `194.0.0` for bigint support
13| // $FlowIssue bigint-unsupported
14| know_of: BigInt(1) << BigInt(0),
15| visible: BigInt(1) << BigInt(1),
16| voiced: BigInt(1) << BigInt(2),
17| edit_entries: BigInt(1) << BigInt(3),
18| edit_thread: BigInt(1) << BigInt(4), // EDIT_THREAD_NAME
19| edit_thread_description: BigInt(1) << BigInt(5),
20| edit_thread_color: BigInt(1) << BigInt(6),
21| delete_thread: BigInt(1) << BigInt(7),
22| create_subthreads: BigInt(1) << BigInt(8), // CREATE_SUBCHANNELS
23| create_sidebars: BigInt(1) << BigInt(9),
24| join_thread: BigInt(1) << BigInt(10),
25| edit_permissions: BigInt(1) << BigInt(11),
26| add_members: BigInt(1) << BigInt(12),
27| remove_members: BigInt(1) << BigInt(13),
28| change_role: BigInt(1) << BigInt(14),
29| leave_thread: BigInt(1) << BigInt(15),
30| react_to_message: BigInt(1) << BigInt(16),
31| edit_message: BigInt(1) << BigInt(17),
32| edit_thread_avatar: BigInt(1) << BigInt(18),
33| manage_pins: BigInt(1) << BigInt(19),
34| manage_invite_links: BigInt(1) << BigInt(20),
35| });
^ [1]
Found 2 errorsThe issue being that permissionsToBitmask is typed as (permissions: ThreadPermissionsInfo): bigint where ThreadPermissionsInfo is typed as
export type ThreadPermissionsInfo = {
+[permission: ThreadPermission]: ThreadPermissionInfo,
};and ThreadPermission is typed as:
export type ThreadPermission = $Values<typeof threadPermissions>;
I could go ahead and create new types that parallel the existing ones, but there might be some value in maintaining the symmetry even if we have an extraneous bit in our new minimal encoding?
Comment Actions
rip out membership permission
in hindsight this might've made sense as its own diff, but hopefully fine for now