-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: MemberInfo
- expose when a project member joined the project
#745
Conversation
test-e2e/members.js
Outdated
@@ -25,8 +25,7 @@ test('getting yourself after creating project', async (t) => { | |||
const deviceInfo = manager.getDeviceInfo() | |||
const project = await manager.getProject(await manager.createProject()) | |||
|
|||
const me = await project.$member.getById(project.deviceId) | |||
|
|||
const { joinedAt: _, ...me } = await project.$member.getById(project.deviceId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test the value of joinedAt
somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, was thinking about this, see -> #745 (comment)
@@ -30,6 +30,7 @@ import { ROLES, isRoleIdForNewInvite } from './roles.js' | |||
* @prop {import('./roles.js').Role} role | |||
* @prop {import('@mapeo/schema').DeviceInfo['name']} [name] | |||
* @prop {import('@mapeo/schema').DeviceInfo['deviceType']} [deviceType] | |||
* @prop {import('@mapeo/schema').DeviceInfo['createdAt']} [joinedAt] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can make this required? It seems like it should always be defined, at least theoretically. Might require a larger change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, was going through that. The easiest way I'm thinking of is turning the result (here) into a SetOptional<MemberInfo,'joinedAt'>
when declared. Is there a better type wizardry for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm, I think is basically the same making it an optional field. I always struggle with ts when declaring an object of a type where some members are added some lines later. Is there a way to signal this to the type system?? (basically, 'I know joinedAt
is missing, just look some lines later!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think the issue is that calling this.#coreOwnership.getCoreId
and this.#dataTypes.deviceInfo.getByDocId
can throw. So if that happens then joinedAt
would be undefined
(as with the other optionals of MemberInfo
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess if everything else can be undefined then this is okay too.
test-e2e/members.js
Outdated
@@ -25,8 +25,7 @@ test('getting yourself after creating project', async (t) => { | |||
const deviceInfo = manager.getDeviceInfo() | |||
const project = await manager.getProject(await manager.createProject()) | |||
|
|||
const me = await project.$member.getById(project.deviceId) | |||
|
|||
const { joinedAt: _, ...me } = await project.$member.getById(project.deviceId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all tests fail 'cause they don't expect a joinedAt
wondering options:
- remove
joinedAt
from the comparison and leave it at that - remove
joinedAt
from the comparison but assert that the field should be there (despite not caring about matching values) - another way of getting the expected value for
joinedAt
to add to the expected object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the 2nd option. Maybe we could also check that the timestamp is near the current timestamp?
Looks like there's a test failure, too. |
…xposeCreatedAtToMemberApi
Co-authored-by: Evan Hahn <[email protected]>
should close #744