⚙ D9549 [lib] Introduce basic `permissionsToBitmask` and `hasPermission`

atul created this revision.Oct 19 2023, 9:28 PM

Herald added a subscriber: will. · View Herald TranscriptOct 19 2023, 9:28 PM

atul published this revision for review.Oct 19 2023, 9:28 PM

Harbormaster completed remote builds in B23388: Diff 32235.Oct 19 2023, 9:46 PM

tomek accepted this revision.Oct 20 2023, 8:01 AM

tomek added inline comments.

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? `const minimallyEncodedThreadPermissions: {+[permission: ThreadPermission]: number}`

This revision is now accepted and ready to land.Oct 20 2023, 8:01 AM

What is the plan for the source?

rohan added inline comments.Oct 20 2023, 3:18 PM

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

ashoat added inline comments.Oct 22 2023, 10:39 PM

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

Use BigInt instead of number.

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`

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`

This revision now requires changes to proceed.Oct 24 2023, 5:30 PM

ashoat added inline comments.Oct 24 2023, 5:31 PM

.eslintrc.json
5 ↗	(On Diff #32346)	s/current/curious

Harbormaster completed remote builds in B23446: Diff 32346.Oct 24 2023, 5:32 PM

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

atul added inline comments.Oct 24 2023, 5:45 PM

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

atul attached a referenced file: F835202: 1f63c7.png. (Show Details)Oct 24 2023, 5:45 PM

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.

Accepting, but please remove the membership permission before landing!

.eslintrc.json
5 ↗	(On Diff #32346)	Thanks for explaining

This revision is now accepted and ready to land.Oct 24 2023, 5:52 PM

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 errors

The 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?

You can remove it from threadPermissions as well