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

Support nullable properties #3

Closed
wants to merge 1 commit into from
Closed

Support nullable properties #3

wants to merge 1 commit into from

Conversation

xetxeberria
Copy link
Contributor

When I tried to use this type in one of my projects, I noticed that it does not support nullable properties.

I added a Nullable type inside the Primitives.ts file, and then used it as the generic in Properties, so the new Primitives type looks like this:

type Nullable<T> = T | null;
export type Primitives<T> = ValueObjectValue<Properties<Nullable<T>>>;

To test it, I added a nullable property to Course and create an specific test for this feature. This addition forced me to also change the existing test.

@JavierCane
Copy link
Member

Hi @xetxeberria!

Firstly: Thanks a lot for your contribution. This is something even ourselves will be interested into 😊

We would ask you to please consider two main things:

  • Update this PR to the latest commits available in the main branch. This is important because we changed how we test the Primitives<T> type in order to use the expect-type library in order to gain robustness in the test suite. We did these changes before opening the PR but sorry in any case for the inconveniences or the extra work it can imply on your side 🙏

Regarding this consideration, here you have how it could be implemented:

  it("should support nullable properties", () => {
    type actualPrimitives = Primitives<SomeNewClass>;

    type expectedPrimitives = {
        readonly prop1: string;
        readonly prop2: string | null;
    };

    expectTypeOf<actualPrimitives>().toEqualTypeOf<expectedPrimitives>();
  });
  • Because of an error on our side, we were not enabling the strictNullCheck tsconfig parameter for the tests/ folder. That is, the previous tsconfig configuration until reviewing this PR was only applying to the code inside the src/ folder. We discovered this because reading the code of your PR we were not actually understanding how it could successfully work. That is, if we set the Nullable<T> type for the actual T generic as you propose, we would be setting that the thing that could be null would be the entire entity. For instance, if the evidence for the T generic is the Course one, with Nullable<Course> we would be stating that the entire Course could be null, not its properties, which we understand was your initial purpose.

That is, we should modify the transformation type ValueObjectValue in order to include in it the Nullable providing support to objects such as the previous one, that is:

    type expectedPrimitives = {
        readonly prop1: string;
        readonly prop2: string | null;
    };
  • Isolate tests: It would be nice to do not mix different features in the same test case. That is, if we add a nullable property inside the current Course entity, the very first test case with the base happy path case would fail for instance if we change the Nullable behaviour. Suggestion: Feel free to create another entity as we are already doing with the test case checking out value object values flatten, and avoid adding nullable properties to the current entities 🤟

Again, thanks a lot for your proposal, we had a good time reviewing it and understanding why it wasn't working as expected until discovered the tsconfig configuration mistake on our side 😅!

🙌🎩👏

@xetxeberria
Copy link
Contributor Author

Thanks for the feedback @JavierCane!

My fault, I noticed your changes once I submitted the PR 😅

I'm making a new one using the latest version and with dedicated tests, feel free to delete this one.

Thanks again for the feedback and the Typescript advanced course, I am enjoying it a lot! 😁🤟

This pull request was closed.
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.

None yet

2 participants