-
Notifications
You must be signed in to change notification settings - Fork 806
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: typescript utilities #2599
Conversation
tien
commented
May 9, 2024
•
edited
Loading
edited
- Improve structures type definitions
b5598f3
to
94fbdf0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2599 +/- ##
============================================
+ Coverage 76.16% 80.33% +4.16%
============================================
Files 1085 27 -1058
Lines 65189 5003 -60186
Branches 7289 0 -7289
============================================
- Hits 49651 4019 -45632
+ Misses 12830 781 -12049
+ Partials 2708 203 -2505 ☔ View full report in Codecov by Sentry. |
34ad293
to
2c5e2d8
Compare
If I'm not mistaken, then this PR does two things which are mostly separate from each other, right? It extends the type definitions for existing types like I think it would be good to separate these two things from each other. Could you please create issues for them in our JIRA? Our usual development workflow is to first have an issue there which describes the bug / proposed improvement and afterwards create the PR. This also makes it possible to first discuss the issue before actually implementing it. In this case, I'm not 100% about the added utility method. It sounds to me like it's a first step in the direction of the Sugar Plugin for Groovy and I'm not sure whether we should implement such syntactic sugar for the GLVs like gremlin-javascript. Sorry for the slight push back here in general. Your Typescript improvements are more than welcome, but I think they are difficult for us to review since we simply lack some experience in this area. That is why I think that following this process helps as it makes it easier for other contributors to better understand the improvements before reviewing them. |
Hi @tien, thanks for putting this together. I agree with Florian that for this type of change, it is best to start with a JIRA or dev list post before starting implementation. I do think that consistency between the GLV's is important, but I can also see how this could be a valuable convenience for users. I do think if we are to add this for vertices in JS, we need to do the equivalent for edges. In terms of naming, an alternative worth considering might be The overall implementation looks good to me, although I wonder if the need to provide explicit cardinality schemas for anything other than singles will limit the convenience upside here. It would be cool if there was an option to try and detect the cardinality of each property, and only force a conversion if the user explicitly specifies an override. Granted this would currently be limited to just single and list, as set is currently blocked by TINKERPOP-3011 In general, if this is something that JS users want in the GLV, then I'm willing to put aside some of the cross-GLV consistency concerns here. I think it's worth a quick dev list thread just to ensure the design is aligned with the community. |
2c5e2d8
to
9ec2032
Compare
Hey guys, thanks for the feedbacks, yes that make sense. So I've reduced the scope of the PR change to just improving the TypeScript definition for graph structures: vertex, edge, etc |
I mainly included the utility function as a demonstration of what a strong TypeScript definition can enable. As for the benefit of having an utility function like that, with TypeScript I have to do a lot of boilerplates just to extract basic values from vertices, i.e: const vertices = await g.V().hasLabel("person").toList<PersonVertex>();
const people = vertices.map(vertex => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const age = vertex.properties.age.at(0)!.value;
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const name = vertex.properties.name.at(0)!.value;
const addresses = vertex.properties.address.map(x => x.value);
return {
age,
name,
addresses
};
}); which with the utility function can be simplified to const vertices = await g.V().hasLabel("person").toList<PersonVertex>();
const people = vertices.map(vertex => parseVertex(vertex, { address: "list" })); |
I'm happy to pursue this with a Jira ticket or via the dev list as mentioned if you guys think a utility like this is valuable to be included in the core gremlin lib. |
9ec2032
to
459bee2
Compare
Hi @tien, Perhaps it makes sense to make an example from this code? Or some utility class? this is really beautiful code, but my opinion is VOTE+0 |
Hey @vkagamlyk, as per my comment here I've removed all none GLV's feature parity changes, so this PR will just be improving the TypeScript definition. No feature changes are involved. This is the utility function that I've now removed, demonstrating the benefit of strong type definition export const parseVertex = <
TVertex extends Vertex,
TCardinality extends { [P in keyof NonNullable<TVertex['properties']>]?: 'single' | 'list' | 'set' | undefined },
>(
vertex: TVertex,
cardinality: TCardinality = {} as TCardinality,
): {
[P in keyof NonNullable<TVertex['properties']>]: (typeof cardinality)[P] extends 'single'
? NonNullable<TVertex['properties']>[P][0]['value']
: (typeof cardinality)[P] extends 'list'
? Array<NonNullable<TVertex['properties']>[P][0]['value']>
: (typeof cardinality)[P] extends 'set'
? Set<NonNullable<TVertex['properties']>[P][0]['value']>
: NonNullable<TVertex['properties']>[P][0]['value'];
} =>
Object.fromEntries(
Object.entries<VertexProperties>(vertex.properties as any).map(([key, value]): any => [
key,
(() => {
switch (cardinality[key]) {
case 'list':
return value.map((x) => x.value);
case 'set':
return new Set(value.map((x) => x.value));
case 'single':
default:
return value[0].value;
}
})(),
]),
) as any;
type ThingVertex = Vertex<"thing", { name: string; alias: string; scores: number }>;
// Raw vertex, for example when calling g.V().next()
const rawVertex: ThingVertex = {
id: 0,
label: 'thing',
properties: {
name: [{ id: 0, label: 'name', key: 'name', value: 'Foo' }],
alias: [
{ id: 0, label: 'alias', key: 'alias', value: 'Bar' },
{ id: 0, label: 'alias', key: 'alias', value: 'Baz' },
],
scores: [
{ id: 0, label: 'score', key: 'score', value: 4 },
{ id: 0, label: 'score', key: 'score', value: 20 },
],
},
};
const vertex = parseVertex(rawVertex, {
// Optional, default to single
name: 'single',
alias: 'list',
scores: 'set',
}); // Fully type safe
console.log(vertex); // Output: { name: 'Foo', alias: ['Bar', 'Baz'], scores: Set [ 4, 20 ] } |
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 just tried to review this, but I have to say that my TypeScript knowledge is quite limited. So most of my comments are really only questions of understanding.
In some cases, your type definitions are also more strict than what we have in other GLVs, and especially in Java which should be the reference.
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/type-serializers.ts
Outdated
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts
Outdated
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts
Show resolved
Hide resolved
459bee2
to
9c9102b
Compare
export class Vertex<T extends Record<string, any> = Record<string, any>> extends Element { | ||
export class Vertex< | ||
TLabel extends string = string, | ||
TProperties extends Record<string, any> = Record<string, any>, |
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.
This looks like good work, but I don't know TypeScript. Quick question to get oriented: is optionality in property types supported, and if so, how? I.e. can you declare a certain vertex type to have a mandatory string-valued "name" property, and another vertex type to have an optional string-valued "name" property?
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.
Yes, you can do something like this:
type Human = Vertex<"person", { name: string; }>;
type NewBorn = Vertex<"person", { name?: string; }>;
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.
Thanks a lot for your explanations and your patience here, @tien!
VOTE +1
Thank you for the contribution! As lazy consensus has been reached, I'll be merging this PR today if there are no further comments. One small note, it does look like this PR is missing a CHANGELOG entry, which I'm happy to add it after merging as CTR. |