Skip to content

Adds optional generic parameter to Node and Relationship #780

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

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

1pxone
Copy link
Contributor

@1pxone 1pxone commented Oct 5, 2021

This feature helps to pass typings of Node and Relationship properties.

  • Generic is optional
  • No more local overrides are needed
import { IUser } from './interfaces/user.inteface';
import { Node } from 'neo4j-driver';

export class User {
    constructor(private readonly node: Node<number, IUser>) {}

    getPassword(): string {
        return this.node.properties.password;
    }
    // ...
}

@bigmontz
Copy link
Contributor

bigmontz commented Oct 7, 2021

Hey @1pxone,

thanks for you contribution. For your PR be able to be merge, you need to read and sign Neo4j's CLA (https://neo4j.com/developer/cla/).

Apart of this, the PR looks really good. Just missing some unit tests to check if the type inference is happening as expected.

@1pxone
Copy link
Contributor Author

1pxone commented Oct 7, 2021

Hey @1pxone,

thanks for you contribution. For your PR be able to be merge, you need to read and sign Neo4j's CLA (https://neo4j.com/developer/cla/).

Apart of this, the PR looks really good. Just missing some unit tests to check if the type inference is happening as expected.

@bigmontz Thanks for your reply. I've just sent signed CLA. Can you give some details about tests? Where should I put them?

@bigmontz
Copy link
Contributor

bigmontz commented Oct 7, 2021

We have typing tests defined for this class at test/types/graph-types.test.ts. You can run these tests by exec the command gulp run-ts-declaration-tests in the repo root. This tests check if the file have a valid TS syntax with the correct types.

Then, there you could create a Node, Relationship, etc with the property interface being supplied and check if you can navigate into the properties as expected (like in the example you provide).

@1pxone
Copy link
Contributor Author

1pxone commented Oct 8, 2021

We have typing tests defined for this class at test/types/graph-types.test.ts. You can run these tests by exec the command gulp run-ts-declaration-tests in the repo root. This tests check if the file have a valid TS syntax with the correct types.

Then, there you could create a Node, Relationship, etc with the property interface being supplied and check if you can navigate into the properties as expected (like in the example you provide).

@bigmontz I've got it, thank you for the clarification. I've just pushed some tests, check it, please :)

Copy link
Contributor

@bigmontz bigmontz left a comment

Choose a reason for hiding this comment

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

🥇

Thanks @1pxone, It will be merged as soon as we get a green pipeline.

@bigmontz
Copy link
Contributor

bigmontz commented Oct 12, 2021

Hi @1pxone,

I've taken a look at your CLA signing and it's missing the github username there after your e-mail. You put your name instead.

Could you please resend with the correction?

More details in: https://neo4j.com/developer/cla/

@1pxone
Copy link
Contributor Author

1pxone commented Oct 12, 2021

@1pxone

@bigmontz Hey! I've just sent it again with correction :)

@bigmontz
Copy link
Contributor

Hi @1pxone,

We are having an issue in linking your commits with your Github user. It seems like you commits doesn't have the correct e-mail linked. Could you amend your commits to add your email to them?

See: https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup

Thanks.

@1pxone 1pxone force-pushed the feature-properties-typings branch from cc67701 to caf7f93 Compare October 14, 2021 10:16
@1pxone
Copy link
Contributor Author

1pxone commented Oct 14, 2021

Hi @1pxone,

We are having an issue in linking your commits with your Github user. It seems like you commits doesn't have the correct e-mail linked. Could you amend your commits to add your email to them?

See: https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup

Thanks.

@bigmontz Ah my fault. I've forgotten to change config before diving to Open Source :) Now it seems correct.

@bigmontz
Copy link
Contributor

Hi @1pxone,

somehow dependabot managed to add a commit in your PR and it is being blocked by the whitelist check because of this. Could you drop the commit?

5c0d8fb

Sorry for the backing and forth process

@1pxone 1pxone force-pushed the feature-properties-typings branch from cab9df4 to 9a529b7 Compare October 14, 2021 15:16
@1pxone
Copy link
Contributor Author

1pxone commented Oct 14, 2021

Hi @1pxone,

somehow dependabot managed to add a commit in your PR and it is being blocked by the whitelist check because of this. Could you drop the commit?

5c0d8fb

Sorry for the backing and forth process

@bigmontz Finally I managed it lol :)

@bigmontz
Copy link
Contributor

Thanks, @1pxone.

I will ping here when it get released.

@bigmontz bigmontz merged commit 9c30c13 into neo4j:4.3 Oct 14, 2021
@bigmontz
Copy link
Contributor

Type improvement release in version 4.3.4.

Release notes: https://github.com/neo4j/neo4j-javascript-driver/wiki/4.3-changelog#434

cc: @1pxone

bigmontz pushed a commit that referenced this pull request Oct 15, 2021
 adds optional generic parameter to Node and Relationship
bigmontz pushed a commit that referenced this pull request Oct 18, 2021
 adds optional generic parameter to Node and Relationship
@1pxone 1pxone deleted the feature-properties-typings branch October 20, 2021 22:19
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