Conversation
Co-authored-by: Aaron Goin <[email protected]>
src/types/auth.ts
Outdated
| }; | ||
|
|
||
| // Returned by /auth in newer API versions | ||
| permissions?: string[] | Record<string, boolean>; |
There was a problem hiding this comment.
Does this type actually come back as an array or strings? I don't think it should.
src/permissions.ts
Outdated
| const enabled = Object.keys(source as Record<string, unknown>).filter( | ||
| (k) => Boolean((source as any)[k]), | ||
| ); |
There was a problem hiding this comment.
If the type gets updated in the types/auth.ts file then you shouldn't need to cast here. And you could probably then just write this like:
| const enabled = Object.keys(source as Record<string, unknown>).filter( | |
| (k) => Boolean((source as any)[k]), | |
| ); | |
| const enabled = Object.keys(source).filter((k) => !!source[k]); |
There was a problem hiding this comment.
using !!source[k] could be unsafe because any truthy non-boolean value (e.g. "none" or 1) would be incorrectly treated as enabled, right? what about const enabled = Object.keys(source).filter((k) => source[k] === true), so only explicitly enabled permissions are included
aarongoin
left a comment
There was a problem hiding this comment.
Nice. Just a couple little nits and improvements.
aarongoin
left a comment
There was a problem hiding this comment.
Okay this looks good. In order to deploy it though we're going to need to bump the package version (probably just a patch level bump since we're still pre-version 1) in the package.json and then npm install to update the lock file. Then once those changes are in the PR we can merge and the CI/CD workflow will handle actually bundling and deploying the new version.
https://github.com/polyapi/poly-alpha/issues/3482