Fix/test coverage#1
Conversation
Removes the unused uuid package dependency to reduce bundle size and minimize external dependencies. Reformats code throughout the service to improve readability by: - Organizing imports alphabetically and grouping related imports - Breaking long function signatures across multiple lines - Improving consistency in constructor parameter formatting - Applying consistent multiline formatting for object literals Expands test coverage to include: - Event emission verification for tag lifecycle operations - Edge cases for type-filtered tag queries - Static instance lifecycle management - Fallback behavior when decorator metadata is missing
Formats code examples in README for better readability by adding proper line breaks and indentation to multi-line configurations and method calls. Aligns table columns in documentation to improve visual consistency across all API reference tables. Replaces empty interface with Record type for TaggableModuleOptions to better represent an empty object type. Uses findOneByOrFail instead of findOne for tag retrieval to ensure proper error handling when tag is not found. Removes credits section from documentation.
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving test coverage around the tagging service/mixin behavior (including events), while also tightening tag uniqueness and aligning docs/dependencies with the current feature set.
Changes:
- Expanded
TaggableServicetest coverage (options, type filtering, lifecycle, decorator fallback, and event emission). - Added a new
TaggableMixinspec suite and tests forTaggableNotInitializedException. - Updated core library pieces:
TaggableServiceevent emission behavior,TagEntitycomposite uniqueness,TaggableModuleOptionstyping, and README/dependencies cleanup.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/taggable.service.spec.ts | Adds coverage for getOptions, typed lookup, lifecycle cleanup, decorator fallback, and event emission behavior. |
| test/taggable.mixin.spec.ts | New tests for mixin methods and initialization error behavior. |
| src/taggable.service.ts | Refactors constructor + changes attach event emission to always load the tag (now via findOneByOrFail). |
| src/interfaces/taggable-options.interface.ts | Changes public options type to disallow configuration. |
| src/entities/tag.entity.ts | Adds composite unique index on (slug, type). |
| package.json | Removes uuid dependency and @types/uuid dev dependency. |
| README.md | Formatting updates and removes configuration/credits sections to match current API surface. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { DynamicModule } from "@nestjs/common"; | ||
|
|
||
| export interface TaggableModuleOptions { | ||
| tableName?: string; | ||
| pivotTableName?: string; | ||
| } | ||
| export type TaggableModuleOptions = Record<string, never>; | ||
|
|
||
| export interface TaggableAsyncOptions { | ||
| imports?: (DynamicModule | Promise<DynamicModule> | any)[]; | ||
| useFactory: (...args: any[]) => TaggableModuleOptions | Promise<TaggableModuleOptions>; | ||
| useFactory: ( |
There was a problem hiding this comment.
TaggableModuleOptions is a public exported type (re-exported from src/index.ts) and this change removes the previously supported tableName/pivotTableName shape by making it Record<string, never>. If consumers were using these options, this is a breaking API change; consider keeping an (even if unused) backwards-compatible interface or documenting the breaking change (e.g., changelog / major version bump) and updating TaggableModule.forRoot signature accordingly.
| const pivot = this.taggableRepo.create({ | ||
| tagId, | ||
| taggableType, | ||
| taggableId: entityId, | ||
| }); | ||
| await this.taggableRepo.save(pivot); | ||
|
|
||
| const tag = await this.tagRepo.findOne({ where: { id: tagId } }); | ||
| if (tag) { | ||
| this.emit(TAGGABLE_EVENTS.TAG_ATTACHED, { tag, taggableType, taggableId: entityId }); | ||
| } | ||
| const tag = await this.tagRepo.findOneByOrFail({ id: tagId }); | ||
| this.emit(TAGGABLE_EVENTS.TAG_ATTACHED, { | ||
| tag, | ||
| taggableType, | ||
| taggableId: entityId, | ||
| }); |
There was a problem hiding this comment.
attachTag() saves the pivot before verifying the tag exists, and then findOneByOrFail can throw. If the tag ID is invalid (or FK constraints are off), this can leave an orphaned pivot row and makes the method partially succeed before failing. Fetch/validate the tag (or wrap both operations in a transaction) before inserting the pivot, and only emit the event after the whole operation succeeds.
| it("should throw TaggableNotInitializedException when service is not available", () => { | ||
| const post = new MixinPost(); | ||
| (post as any).id = "fake-id"; | ||
| expect(() => post.attachTag("fake-tag")).rejects.toThrow(TaggableNotInitializedException); |
There was a problem hiding this comment.
This test uses .rejects with a function wrapper (expect(() => post.attachTag(...)).rejects...). In Vitest/Jest, .rejects expects a Promise; wrapping returns a function and the assertion won't behave as intended. Use await expect(post.attachTag("fake-tag")).rejects.toThrow(...) (or return expect(...)) so the rejection is actually asserted.
| it("should throw TaggableNotInitializedException when service is not available", () => { | |
| const post = new MixinPost(); | |
| (post as any).id = "fake-id"; | |
| expect(() => post.attachTag("fake-tag")).rejects.toThrow(TaggableNotInitializedException); | |
| it("should throw TaggableNotInitializedException when service is not available", async () => { | |
| const post = new MixinPost(); | |
| (post as any).id = "fake-id"; | |
| await expect(post.attachTag("fake-tag")).rejects.toThrow(TaggableNotInitializedException); |
| import { TagEntity } from "../src/entities/tag.entity"; | ||
| import { TaggableEntity } from "../src/entities/taggable.entity"; | ||
| import { Taggable } from "../src/decorators/taggable.decorator"; | ||
| import { TaggableModule } from "../src/taggable.module"; |
There was a problem hiding this comment.
Unused import: TaggableModule is added but never referenced in this spec file. This will fail no-unused-vars/@typescript-eslint/no-unused-vars under the current ESLint config. Remove the import (or use it if intended).
| import { TaggableModule } from "../src/taggable.module"; |
| @Entity("tags") | ||
| @Index(["slug", "type"], { unique: true }) | ||
| export class TagEntity { |
There was a problem hiding this comment.
The unique index on (slug, type) will not prevent duplicate slugs when type is NULL in many SQL databases (e.g., Postgres/MySQL allow multiple NULLs in a unique index). If the intent is to guarantee uniqueness for untyped tags as well, consider making type non-nullable with a sentinel default (e.g., empty string) or adding an additional constraint/index strategy for the NULL case.
No description provided.