Skip to content
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: omitServerProps helper function #277

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gmaclennan
Copy link
Member

I needed a more flexible version of valueOf for removing server-managed props from a Mapeo Doc. In the front-end we are modifying some props such as replacing refs with actual docs, but we still need to be able to strip server properties when editing.

I decided to create a separate function since this is subtly different from valueOf, and the name valueOf isn't great anyway.

@gmaclennan gmaclennan self-assigned this Dec 3, 2024
@gmaclennan gmaclennan requested a review from EvanHahn December 3, 2024 21:52
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I was initially surprised to see that the word "server" meant "managed by the backend"...maybe there's a better name for this?
  2. I admit I don't fully understand the motivation for this change but I trust that y'all have discussed it and have a good reason.
  3. Do we have a plan to deprecate valueOf?

export function omitServerProps<TDoc extends MapeoCommon>(
doc: TDoc & { forks?: string[] }
) {
return excludeKeys(doc, keysToExclude)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use our existing omit helper (and maybe remove the new filter-obj dependency)?

@@ -79,6 +82,10 @@ export function parseVersionId(versionId: string): VersionIdObject {
return { coreDiscoveryKey, index }
}

/**
* Get the value of a document, excluding server-managed properties.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is a little unclear because it makes it look like it should do the same thing as the other function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants