Replace ts keyof transformer usage with runtime code #3413
Conversation
… esm classes This works as long as there is no type mapping in the inheritance chain. This is fine for this single use case. And this can also go away once we finish Gel migration.
This enum value needs to be lazily referenced, so the actual enum defers creation until after files have had a chance to run their side effects to add their members.
JS first calls ownKeys and then looks at the property descriptors to see if the key is enumerable. So we need to forward the descriptors as well for this to work.
📝 WalkthroughWalkthroughThis update introduces several structural and type system changes across the codebase. The main focus is the replacement of string literal union types and dynamic key extraction utilities (such as Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/progress-report/workflow/transitions.ts (1)
94-95: Refactored TransitionName from type-only to enum-based implementationThe
TransitionNamehas been refactored from a simple TypeScript type to a proper enum created withmakeEnum. This provides both compile-time and runtime benefits - you can now useTransitionNameas a value, not just as a type.Consider adding a concise comment explaining this pattern for future developers, especially if this change is part of a broader refactoring effort across the codebase:
+// Convert TransitionName from a simple type to an enum that can be used as both type and value export type TransitionName = EnumType<typeof TransitionName>; export const TransitionName = makeEnum(entries(Transitions).map(([k]) => k));src/components/product/dto/producible.dto.ts (1)
54-63: Refactored ProducibleType to use lazy initialization patternThe implementation has been redesigned to:
- Use lazy initialization with
lazyRef- Cache the created enum instance
- Allow dynamic extension via the
ProducibleTypeEntriesset- Register the enum with GraphQL schema eagerly
This is a significant improvement in extensibility, allowing modules to register their producible types in a decentralized manner while maintaining type safety.
Consider adding a brief comment explaining the extensibility pattern for developers unfamiliar with this approach:
// Augment this with each implementation of Producible via declaration merging // eslint-disable-next-line @typescript-eslint/no-empty-interface export interface ProducibleTypeEntries {} export const ProducibleTypeEntries = new Set<string>(); +// Lazy enum that builds from all registered ProducibleTypeEntries +// Other modules can augment this enum by adding to ProducibleTypeEntries export type ProducibleType = EnumType<typeof ProducibleType>; export const ProducibleType = lazyRef( (): MadeEnum<keyof ProducibleTypeEntries> => (realProducibleType ??= makeEnum({ values: ProducibleTypeEntries as Iterable<keyof ProducibleTypeEntries>, })), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
package.json(1 hunks)src/common/lazy-ref.ts(1 hunks)src/components/authorization/policy/actions.ts(1 hunks)src/components/authorization/policy/executor/all-permissions-view.ts(3 hunks)src/components/authorization/policy/executor/policy-dumper.ts(1 hunks)src/components/authorization/policy/policy.factory.ts(1 hunks)src/components/engagement/dto/create-engagement.dto.ts(4 hunks)src/components/ethno-art/dto/ethno-art.dto.ts(1 hunks)src/components/film/dto/film.dto.ts(1 hunks)src/components/product/dto/producible.dto.ts(2 hunks)src/components/product/dto/product.dto.ts(2 hunks)src/components/progress-report/workflow/dto/workflow-transition.dto.ts(1 hunks)src/components/progress-report/workflow/transitions.ts(2 hunks)src/components/story/dto/story.dto.ts(1 hunks)src/core/config/config.service.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/components/story/dto/story.dto.ts (1)
src/components/product/dto/producible.dto.ts (2)
ProducibleTypeEntries(51-51)ProducibleTypeEntries(52-52)
src/components/product/dto/product.dto.ts (1)
src/components/product/dto/producible.dto.ts (2)
ProducibleTypeEntries(51-51)ProducibleTypeEntries(52-52)
src/components/film/dto/film.dto.ts (1)
src/components/product/dto/producible.dto.ts (2)
ProducibleTypeEntries(51-51)ProducibleTypeEntries(52-52)
src/components/progress-report/workflow/transitions.ts (1)
src/common/index.ts (2)
EnumType(2-2)makeEnum(2-2)
src/components/authorization/policy/executor/policy-dumper.ts (1)
src/components/authorization/policy/actions.ts (8)
ResourceAction(6-6)ResourceAction(7-7)PropAction(12-12)PropAction(13-13)ChildSingleAction(30-30)ChildSingleAction(31-31)ChildListAction(24-24)ChildListAction(25-25)
src/components/authorization/policy/policy.factory.ts (1)
src/components/authorization/policy/actions.ts (4)
ChildListAction(24-24)ChildListAction(25-25)ChildSingleAction(30-30)ChildSingleAction(31-31)
src/components/ethno-art/dto/ethno-art.dto.ts (1)
src/components/product/dto/producible.dto.ts (2)
ProducibleTypeEntries(51-51)ProducibleTypeEntries(52-52)
src/components/authorization/policy/executor/all-permissions-view.ts (2)
src/components/authorization/policy/actions.ts (2)
AnyAction(37-37)AnyAction(38-42)src/common/index.ts (2)
EnumType(2-2)makeEnum(2-2)
src/components/authorization/policy/actions.ts (1)
src/common/index.ts (2)
EnumType(2-2)makeEnum(2-2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: lint
- GitHub Check: Clean
- GitHub Check: Generate (base)
- GitHub Check: Unit
🔇 Additional comments (30)
package.json (1)
52-52: Package version update aligns with transformer removal strategyThe version updates for
@seedcompany/common(to >=0.19.1) and@seedcompany/nest(to ^1.6.1) support the PR's goal of replacing the TypeScript keyof transformer with runtime code. These updated packages provide the necessary enum utilities and related features used throughout the refactoring.Also applies to: 54-54
src/components/progress-report/workflow/dto/workflow-transition.dto.ts (1)
4-4: Import path updated for consistencyThe more specific import path for
ProgressReportStatusaligns with the broader refactoring to use explicit enum types instead of relying on the TypeScript keyof transformer.src/components/story/dto/story.dto.ts (2)
12-14: Story type registration implements the new runtime approachThe code now imports
ProducibleTypeEntriesand registers 'Story' as a valid producible type at runtime, effectively replacing the static type extraction previously handled by the TypeScript keyof transformer. This is consistent with the PR's objective of moving away from transformer usage to runtime code.
15-19: Module augmentation completes the type registration patternThe TypeScript module augmentation ensures type safety by declaring 'Story' in the
ProducibleTypeEntriesinterface. This dual approach (runtime registration + type augmentation) maintains both runtime functionality and compile-time type checking.src/components/film/dto/film.dto.ts (2)
12-15: Import formatting and extension for runtime type registrationThe import statement has been restructured and extended to include
ProducibleTypeEntries, preparing for the runtime registration of the Film type. This is consistent with the PR's goal of replacing transformer-based approaches with runtime code.
17-22: Film type registered using the new patternSimilar to the Story DTO, this code registers 'Film' as a valid producible type at runtime and augments the TypeScript module to maintain type safety. This implementation correctly follows the new pattern established for producible types across the codebase.
src/components/ethno-art/dto/ethno-art.dto.ts (2)
12-15: Import updated to register EthnoArt as a producible type.The import was expanded to include
ProducibleTypeEntrieswhich is necessary for the next step where EthnoArt is registered as a producible type. This aligns with the broader refactoring approach replacing the static type extraction with runtime registration.
17-17: EthnoArt successfully registered as a producible type.The code now adds 'EthnoArt' to the ProducibleTypeEntries set, properly registering it at runtime as a valid producible type. This matches the module augmentation in lines 18-22 and aligns with the pattern used for other producible types in the codebase.
src/components/product/dto/product.dto.ts (2)
30-35: Import expanded to support runtime type registration.The import statement was updated to include
ProducibleTypeEntries, allowing the file to register product types at runtime rather than relying on the TypeScript keyof transformer. This is consistent with the overall refactoring strategy.
269-271: Product types correctly registered with ProducibleTypeEntries.The code now registers three product types ('DirectScriptureProduct', 'DerivativeScriptureProduct', and 'OtherProduct') with the ProducibleTypeEntries set. This ensures they're recognized as valid producible types at runtime and matches the module augmentation in lines 272-278.
src/core/config/config.service.ts (2)
16-16: Value import replaces type-only import for TransitionName.Changed from importing only the type to importing the actual enum value. This is necessary for the conversion from using the TypeScript keyof transformer to using runtime code.
135-135: Replaced keyof transformer with direct enum reference.The code now directly uses the
ProgressReportTransitionNameenum instead of usingkeysOf<Record<ProgressReportTransitionName, ''>>(). This eliminates the dependency on the ts-transformer-keys library for this case and aligns with the PR objective of replacing keyof transformer usage.src/components/authorization/policy/policy.factory.ts (2)
316-316: Replaced keyof extraction with direct enum reference.The code now directly assigns the appropriate enum (
ChildListActionorChildSingleAction) instead of using thekeysOftransformer to extract keys at runtime. This simplifies the code and removes a dependency on the ts-transformer-keys library.
320-321: Spread operator correctly converts enum to array.The call to
picknow uses[...childActions]to spread the enum values into an array, as required by the function. This is an appropriate way to convert an enum object to an array of its values for use with thepickfunction.src/common/lazy-ref.ts (1)
26-28: Good enhancement to support property descriptor reflection!This addition of the
getOwnPropertyDescriptortrap to the proxy handler enhances thelazyReffunction's completeness. It allows for proper handling of reflective operations likeObject.getOwnPropertyDescriptor, which is likely needed by frameworks or utilities that inspect object properties.src/components/progress-report/workflow/transitions.ts (2)
1-2: Updated imports for new enum implementationThe added imports are necessary for the new enum implementation pattern that replaces the string literal union type.
5-6: More specific import pathsThe import paths have been updated to more specific module paths, which is good for clarity and maintainability.
src/components/authorization/policy/executor/policy-dumper.ts (2)
190-191: Using ResourceAction enum directly instead of dynamic key extractionThe code now uses the
ResourceActionenum directly inmapValues.fromListinstead of dynamically extracting keys. This improves type safety and makes the dependencies explicit.
195-197: Using action enums directly for property and child actionsSimilar to the previous change, the code now uses the enum objects directly (
PropAction,ChildSingleAction,ChildListAction) instead of dynamic key extraction. This is consistent with the pattern applied elsewhere in the codebase.src/components/product/dto/producible.dto.ts (2)
1-7: Updated imports for enhanced enum implementationAdded import for
registerEnumTypefrom GraphQL and other related utilities needed for the new lazy enum implementation.
52-52: Added mutable set for dynamic producible type registrationThe
ProducibleTypeEntriesset provides a central registry for producible types, allowing different modules to contribute their own types.src/components/authorization/policy/executor/all-permissions-view.ts (3)
43-43: Good refactoring to use the newly defined enum valuesThe change from dynamic key extraction to using
CompatAction.valuesimproves type safety and eliminates the dependency on the TypeScript keyof transformer, aligning with the PR objectives.
77-84: Well-designed enum-based compatibility layerGood approach creating the
CompatActionenum that combines both new action types and legacy "canXxx" prefixed variants. TheasLegacyActionhelper function encapsulates the transformation logic nicely, making the code more maintainable.
86-99: Clean implementation of compatibility mappingsThe updated implementation of
compatMapusingmapValues.fromListwith enum values provides a more robust solution than the previous dynamic key extraction approach. This maintains the same functionality while removing dependencies on TypeScript transformers.src/components/engagement/dto/create-engagement.dto.ts (3)
19-19: Good inheritance refactoringThe change to extend
DataObjectprovides additional functionality to theCreateEngagementclass. This is a positive architectural improvement.
40-44: Runtime property extraction approach needs clarificationThe implementation of static
Propsusing runtime property extraction is a good replacement for the TypeScript keyof transformer. However, the warning comment suggests potential limitations with inheritance type mapping.Can you clarify under what conditions this approach might not work with inheritance type mapping? This seems like an important constraint that should be documented.
87-91: Consistent implementation of Props extractionGood consistency in applying the same pattern to both engagement types, using the same runtime property extraction approach.
src/components/authorization/policy/actions.ts (3)
1-14: Well-structured enum-based actions implementationThe refactoring of action types from string literal unions to enum-based types using
makeEnumandEnumTypeis a significant improvement. This approach provides better type safety and runtime capabilities while removing dependencies on TypeScript transformers.
18-32: Consistent enum pattern across action typesGood consistency in applying the same enum pattern to all action types. The clear separation between different categories of actions (resource, property, child relationships) makes the code more maintainable and self-documenting.
37-42: Elegant composition of action typesUsing spread syntax to compose the
AnyActionenum from other action enums is elegant and maintainable. This clearly communicates thatAnyActionis a union of the other action types while providing all the benefits of the enum approach.
This removes all the usage of this transformer (besides the Props/SecuredProps which will be addressed separately).
This transformer prevents us from using different compilers.