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

Enable strictNullChecks for Better Type Safety #35

Open
elersong opened this issue Jul 13, 2024 · 0 comments
Open

Enable strictNullChecks for Better Type Safety #35

elersong opened this issue Jul 13, 2024 · 0 comments
Labels
from_willyovale An issue described in original project, but never implemented

Comments

@elersong
Copy link
Owner

Description

The findById method in BaseFirestoreRepository is expected to return Promise<T | null>. However, the generated TypeScript declaration file shows Promise<T>, losing the benefit of type safety. This issue is likely due to strictNullChecks being set to false. Enabling strictNullChecks and fixing the resulting errors will improve type safety throughout the library.

Steps to Reproduce

  1. Check the findById method in BaseFirestoreRepository.
  2. Observe that it is expected to return Promise<T | null>.
  3. Build the library and check the generated declaration file.
  4. Notice that the return type is Promise<T> instead of Promise<T | null>.

Expected Behavior

The findById method should correctly reflect Promise<T | null> in the generated declaration file, ensuring proper type safety.

Actual Behavior

The findById method's return type is Promise<T> in the generated declaration file, losing the benefit of type safety.

Acceptance Criteria

  • Enable strictNullChecks in the TypeScript configuration.
  • Fix any resulting errors to ensure the library builds successfully.
  • Ensure the correct return types are reflected in the generated declaration files.

Additional Context

  • March 18, 2022: Initial issue raised about the incorrect return type in the generated declaration file.
  • April 7, 2022: Acknowledgment of the issue and the need for help to fix it.

Proposed API Changes

  1. Enable strictNullChecks:

    • Update the tsconfig.json file to enable strictNullChecks.
    {
      "compilerOptions": {
        "strictNullChecks": true,
        // Other compiler options...
      }
    }
  2. Fix Resulting Errors:

    • Address the errors that occur when enabling strictNullChecks to ensure the library builds successfully.
    // Example fix for the findById method
    async findById(id: string): Promise<T | null> {
      return this.firestoreColRef
        .doc(id)
        .get()
        .then(d => (d.exists ? this.extractTFromDocSnap(d) : null));
    }
  3. Validate Return Types:

    • Ensure the correct return types are reflected in the generated declaration files.
    // Check the generated declaration file
    declare class BaseFirestoreRepository<T> {
      findById(id: string): Promise<T | null>;
    }
  4. Unit Tests:

    • Add unit tests to validate the correct return types and behavior of methods.
    test('findById should return null when document does not exist', async () => {
      const repo = getRepository(MyEntity);
      const result = await repo.findById('nonexistent-id');
      expect(result).toBeNull();
    });
    
    test('findById should return entity when document exists', async () => {
      const repo = getRepository(MyEntity);
      const entity = new MyEntity();
      entity.id = 'existent-id';
      await repo.create(entity);
    
      const result = await repo.findById('existent-id');
      expect(result).toEqual(entity);
    });

Example Implementation

class MyEntity {
  id: string;
}

@Collection("myEntities")
class MyEntityRepository extends BaseFirestoreRepository<MyEntity> {}

const repo = getRepository(MyEntity);

const entity = new MyEntity();
entity.id = "existent-id";
await repo.create(entity);

const result = await repo.findById("existent-id");
console.log(result); // Output: MyEntity instance or null

Original Issue

@elersong elersong added the from_willyovale An issue described in original project, but never implemented label Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from_willyovale An issue described in original project, but never implemented
Projects
None yet
Development

No branches or pull requests

1 participant