Automagically declare Props/SecuredProps with TS transformer
#3411
Conversation
CarsonF
force-pushed
the
ts-5.8
branch
2 times, most recently
from
cbc6303 to
6fac407
Compare
CarsonF
force-pushed
the
ts-plugin-resources
branch
2 times, most recently
from
d9a10ad to
688a429
Compare
CarsonF
changed the title
Props/SecuredProps with Nest pluginProps/SecuredProps with TS transformer
CarsonF
force-pushed
the
ts-plugin-resources
branch
from
688a429 to
836a733
Compare
CarsonF
force-pushed
the
ts-plugin-resources
branch
4 times, most recently
from
2c8f0a6 to
f1a7179
Compare
They will only be declared in compiled code now.
I'm having a hard time getting the transformer to load & transpile. There are too many different loading variations between Nest, Jest, ts-node, ts-patch. It was giving a cryptic error with gel:gen (ts-node). I don't want to get stuck here because the landscape is going to change as we switch to SWC, vitest, etc. There's possibly a good performance gain to do this anyway, unsure, unmeasured. The JS with jsdocs is _almost_ as good - TS does check it.
CarsonF
force-pushed
the
ts-plugin-resources
branch
from
f1a7179 to
2262ea2
Compare
📝 WalkthroughWalkthroughThis change removes the Changes
Possibly related PRs
✨ 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: 3
🔭 Outside diff range comments (1)
src/components/changeset/dto/changeset.dto.ts (1)
1-29: 💡 Verification agent🧩 Analysis chain
Verify transformer functionality
All changes look good. To ensure the transformer is working correctly, you might want to add a test that verifies the presence of these properties at runtime.
🏁 Script executed:
#!/bin/bash # Verify that the transformer correctly adds Props and SecuredProps to classes # by checking if there are any runtime errors related to missing properties # Look for the EnhancedResource implementation that uses these properties echo "Looking for EnhancedResource implementation..." rg -A 10 "class EnhancedResource" --type ts # Check if there are any tests that verify the presence of Props and SecuredProps echo "Checking for tests that verify Props and SecuredProps..." rg -A 5 "Props|SecuredProps" --type ts --glob "*.spec.ts"Length of output: 1042
Add runtime tests for transformer-generated properties
We haven’t found any existing specs that assert the transformer is adding the
PropsandSecuredPropsfields to your resources at runtime. Please add a test suite (for example,src/common/resource.dto.spec.ts) that:
- Imports
EnhancedResource(e.g. via a sample resource likeChangeset)- Instantiates it or derives it from your GraphQL object
- Asserts the presence of the
propsandsecuredPropsproperties- Verifies that those objects include the expected fields (
editable,applied, etc.)This will ensure the transformer wiring remains correct as the code evolves.
🧹 Nitpick comments (4)
src/common/resource.dto.ts (1)
76-77: Preferreadonlyarrays for immutabilityThe metadata arrays are intended to be constants that should never be mutated at runtime.
Declaring them asreadonly ...[](orReadonlyArray<string>) communicates this intent to TypeScript and prevents accidental writes.- Props?: string[]; - SecuredProps?: string[]; + readonly Props?: readonly string[]; + readonly SecuredProps?: readonly string[];src/core/resources/plugin/resources.visitor.js (3)
1-1: Avoid importing Nest internals that may break on upgrade
@nestjs/graphql/dist/plugin/utils/ast-utils.jsis an internal path—minor NestJS releases can rename or relocate it without a semver major bump.
Consider copying the tiny helper you need or using a stable public API to protect the transformer from breaking changes.
83-87: Guard against duplicate static membersIf a developer manually adds
static Props(or the transformer is invoked twice in watch mode) the class will end up with duplicate property declarations and a TS compilation error.Add an existence check before prepending new members.
- return this.updateClassMembers(factory, classNode, [ - this.createStaticPropArray(factory, 'Props', classProps), - this.createStaticPropArray(factory, 'SecuredProps', securedProps), - ...classNode.members, - ]); + const needsProps = !classNode.members.some( + (m) => ts.isPropertyDeclaration(m) && m.name.getText() === 'Props', + ); + const needsSecured = !classNode.members.some( + (m) => ts.isPropertyDeclaration(m) && m.name.getText() === 'SecuredProps', + ); + return this.updateClassMembers(factory, classNode, [ + ...(needsProps + ? [this.createStaticPropArray(factory, 'Props', classProps)] + : []), + ...(needsSecured + ? [this.createStaticPropArray(factory, 'SecuredProps', securedProps)] + : []), + ...classNode.members, + ]);
95-108: Emit literal arrays asas constfor stronger typingAppending
as constlets TS treat the array elements as string literals instead of widening tostring, which preserves exact key names for downstream consumers.- factory.createArrayLiteralExpression( - members.map((p) => factory.createStringLiteral(p.getName(), true)), - ), + factory.createAsExpression( + factory.createArrayLiteralExpression( + members.map((p) => factory.createStringLiteral(p.getName(), true)), + ), + factory.createTypeReferenceNode('const'), + ),
📜 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 (64)
package.json(0 hunks)src/app.module.ts(0 hunks)src/common/keysOf.spec.ts(0 hunks)src/common/resource.dto.ts(2 hunks)src/components/authentication/authentication.service.ts(1 hunks)src/components/authorization/assignable-roles.granter.ts(1 hunks)src/components/authorization/dto/assignable-roles.dto.ts(0 hunks)src/components/authorization/dto/beta-features.dto.ts(0 hunks)src/components/budget/dto/budget-record.dto.ts(0 hunks)src/components/budget/dto/budget.dto.ts(0 hunks)src/components/ceremony/dto/ceremony.dto.ts(0 hunks)src/components/changeset/dto/changeset.dto.ts(1 hunks)src/components/comments/dto/comment-thread.dto.ts(0 hunks)src/components/comments/dto/comment.dto.ts(1 hunks)src/components/comments/dto/commentable.dto.ts(0 hunks)src/components/comments/mention-notification/comment-via-mention-notification.dto.ts(0 hunks)src/components/engagement/dto/engagement.dto.ts(0 hunks)src/components/ethno-art/dto/ethno-art.dto.ts(1 hunks)src/components/field-region/dto/field-region.dto.ts(0 hunks)src/components/field-zone/dto/field-zone.dto.ts(0 hunks)src/components/file/dto/file.dto.ts(0 hunks)src/components/file/media/media.dto.ts(0 hunks)src/components/film/dto/film.dto.ts(1 hunks)src/components/funding-account/dto/funding-account.dto.ts(0 hunks)src/components/language/dto/language.dto.ts(0 hunks)src/components/location/dto/location.dto.ts(0 hunks)src/components/notification-system/system-notification.dto.ts(0 hunks)src/components/notifications/dto/notification.dto.ts(1 hunks)src/components/organization/dto/organization.dto.ts(0 hunks)src/components/partner/dto/partner.dto.ts(0 hunks)src/components/partnership/dto/partnership.dto.ts(0 hunks)src/components/periodic-report/dto/periodic-report.dto.ts(0 hunks)src/components/pnp/extraction-result/extraction-result.dto.ts(0 hunks)src/components/post/dto/post.dto.ts(1 hunks)src/components/post/dto/postable.dto.ts(1 hunks)src/components/product-progress/dto/product-progress.dto.ts(0 hunks)src/components/product-progress/dto/variant-progress.dto.ts(0 hunks)src/components/product/dto/producible.dto.ts(0 hunks)src/components/product/dto/product.dto.ts(0 hunks)src/components/progress-report/dto/community-stories.dto.ts(1 hunks)src/components/progress-report/dto/highlights.dto.ts(1 hunks)src/components/progress-report/dto/progress-report.dto.ts(0 hunks)src/components/progress-report/dto/team-news.dto.ts(1 hunks)src/components/progress-report/media/dto/media.dto.ts(0 hunks)src/components/progress-report/variance-explanation/variance-explanation.dto.ts(0 hunks)src/components/progress-report/workflow/dto/workflow-event.dto.ts(0 hunks)src/components/progress-summary/dto/progress-summary.dto.ts(0 hunks)src/components/project-change-request/dto/project-change-request.dto.ts(1 hunks)src/components/project/dto/project.dto.ts(1 hunks)src/components/project/financial-approver/dto/financial-approver.dto.ts(1 hunks)src/components/project/project-member/dto/project-member.dto.ts(0 hunks)src/components/project/workflow/dto/workflow-event.dto.ts(0 hunks)src/components/prompts/dto/prompt-response.dto.ts(0 hunks)src/components/scripture/dto/scripture-range.dto.ts(1 hunks)src/components/story/dto/story.dto.ts(1 hunks)src/components/user/dto/actor.dto.ts(0 hunks)src/components/user/dto/known-language.dto.ts(1 hunks)src/components/user/dto/user.dto.ts(0 hunks)src/components/user/education/dto/education.dto.ts(0 hunks)src/components/user/unavailability/dto/unavailability.dto.ts(1 hunks)src/components/user/user.service.ts(1 hunks)src/core/resources/plugin/index.js(1 hunks)src/core/resources/plugin/resources.visitor.js(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (40)
- package.json
- src/components/authorization/dto/assignable-roles.dto.ts
- src/app.module.ts
- src/components/user/education/dto/education.dto.ts
- src/components/field-zone/dto/field-zone.dto.ts
- src/components/user/dto/actor.dto.ts
- src/components/budget/dto/budget.dto.ts
- src/components/project/workflow/dto/workflow-event.dto.ts
- src/components/progress-report/dto/progress-report.dto.ts
- src/components/progress-report/media/dto/media.dto.ts
- src/components/product-progress/dto/variant-progress.dto.ts
- src/components/funding-account/dto/funding-account.dto.ts
- src/components/progress-summary/dto/progress-summary.dto.ts
- src/components/pnp/extraction-result/extraction-result.dto.ts
- src/components/progress-report/variance-explanation/variance-explanation.dto.ts
- src/components/product/dto/producible.dto.ts
- src/components/location/dto/location.dto.ts
- src/components/authorization/dto/beta-features.dto.ts
- src/components/notification-system/system-notification.dto.ts
- src/components/user/dto/user.dto.ts
- src/components/project/project-member/dto/project-member.dto.ts
- src/components/comments/dto/comment-thread.dto.ts
- src/components/ceremony/dto/ceremony.dto.ts
- src/components/comments/dto/commentable.dto.ts
- src/common/keysOf.spec.ts
- src/components/partnership/dto/partnership.dto.ts
- src/components/budget/dto/budget-record.dto.ts
- src/components/partner/dto/partner.dto.ts
- src/components/organization/dto/organization.dto.ts
- src/components/comments/mention-notification/comment-via-mention-notification.dto.ts
- src/components/file/dto/file.dto.ts
- src/components/progress-report/workflow/dto/workflow-event.dto.ts
- src/components/language/dto/language.dto.ts
- src/components/file/media/media.dto.ts
- src/components/field-region/dto/field-region.dto.ts
- src/components/periodic-report/dto/periodic-report.dto.ts
- src/components/engagement/dto/engagement.dto.ts
- src/components/prompts/dto/prompt-response.dto.ts
- src/components/product/dto/product.dto.ts
- src/components/product-progress/dto/product-progress.dto.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/core/resources/plugin/resources.visitor.js (2)
src/common/resource.dto.ts (2)
securedProps(204-212)name(155-157)src/common/secured-property.ts (1)
isSecured(84-85)
🔇 Additional comments (34)
src/components/authorization/assignable-roles.granter.ts (1)
2-2: Update import path for AssignableRoles
Aligns with the new DTO filename convention by importing from the.dto-suffixed file. All downstream references should be consistent with this change.src/components/user/user.service.ts (1)
17-17: Align AssignableRoles import with DTO rename
The import path now points toassignable-roles.dto, matching the updated file naming scheme. This keeps consistency across authentication and authorization modules.src/components/authentication/authentication.service.ts (1)
25-25: Update AssignableRoles import path
Consistent refactoring of the DTO import to use the.dtosuffix. No further changes required in this file.tsconfig.json (1)
39-39:✅ Verification successful
Add custom transformer plugin
The new local plugin at./src/core/resources/plugin/index.jsreplaces the third-partyts-transformer-keystransformer. Please verify that this path is correct relative to the repo root and that plugin execution order (aftertypescript-transform-paths) is maintained.
🏁 Script executed:
#!/bin/bash # Verify that the custom transformer file exists if [[ ! -f "src/core/resources/plugin/index.js" ]]; then echo "❌ Custom transformer not found at src/core/resources/plugin/index.js" exit 1 else echo "✅ Custom transformer is present." fiLength of output: 122
🏁 Script executed:
#!/bin/bash # Verify tsconfig.json location and list plugin entries if [[ ! -f "tsconfig.json" ]]; then echo "❌ tsconfig.json not found at repo root." exit 1 else echo "✅ tsconfig.json found at repo root." fi echo echo "📋 'plugins' array in tsconfig.json:" awk '/"plugins"/,/\]/' tsconfig.jsonLength of output: 383
tsconfig.json plugin configuration verified
Both the custom transformer file and its execution order are correct:
- Custom transformer exists at
src/core/resources/plugin/index.js.- In
tsconfig.json, it’s listed immediately after"typescript-transform-paths", preserving the intended order.No further changes required.
src/components/user/dto/known-language.dto.ts (1)
2-2:❓ Verification inconclusive
Remove static keys imports per transformer refactor
Dropped thekeysandSecuredPropsimports because the custom transformer now injectsPropsandSecuredPropsat compile time for any class with NestJS GraphQL decorators. Ensure the transformer correctly augments this class by inspecting the compiled output.
🏁 Script executed:
#!/bin/bash # Verify transformer-injected static properties in compiled DTO grep -R "static readonly Props" dist/src/components/user/dto/known-language.js \ && echo "✅ Props injected" || echo "❌ Props missing" grep -R "static readonly SecuredProps" dist/src/components/user/dto/known-language.js \ && echo "✅ SecuredProps injected" || echo "❌ SecuredProps missing"Length of output: 424
🏁 Script executed:
#!/bin/bash set -e # Locate the compiled DTO file file=$(find dist -type f -iname "known-language.js" | head -n1) if [ -z "$file" ]; then echo "❌ Compiled file not found" exit 1 fi echo "Compiled file: $file" # Verify transformer-injected static properties grep -R "static readonly Props" "$file" && echo "✅ Props injected" || echo "❌ Props missing" grep -R "static readonly SecuredProps" "$file" && echo "✅ SecuredProps injected" || echo "❌ SecuredProps missing"Length of output: 200
Remove static keys imports per transformer refactor
Dropped thekeysandSecuredPropsimports because our custom transformer now injectsPropsandSecuredPropsat compile time for any class with NestJS GraphQL decorators. Since the sandbox build artifacts aren’t available here, please:
- Run a local build (e.g.
npm run buildoryarn build).- Inspect the compiled DTO at
dist/src/components/user/dto/known-language.js.- Confirm that it contains:
static readonly Props = …static readonly SecuredProps = …src/components/story/dto/story.dto.ts (2)
2-2: Import simplification looks goodThe removal of the
keysimport fromts-transformer-keysandSecuredPropstype from~/commonaligns with the move to auto-generated props.
14-22: LGTM! Auto-generation of Props/SecuredProps in effectThe removal of static
PropsandSecuredPropsproperties is consistent with the PR's objective to automatically generate these properties via a custom TS transformer. Since this class has the@RegisterResourcedecorator, it will now have these properties generated at compile time.src/components/notifications/dto/notification.dto.ts (2)
3-3: Import cleanup looks goodThe imports have been simplified by removing
keysfromts-transformer-keysandSecuredPropsfrom~/common, which are no longer needed with the auto-generation approach.
7-25: Removal of static Props/SecuredProps properties is correctThe
Notificationclass is decorated with@RegisterResource, which will trigger the automatic generation ofPropsandSecuredPropsproperties by the custom TypeScript transformer, making the manual declarations unnecessary.src/components/film/dto/film.dto.ts (2)
2-2: Import simplification looks goodThe removal of
keys as keysOffromts-transformer-keysandtype SecuredPropsproperly aligns with the new approach of auto-generated properties.
17-25: LGTM! Static Props/SecuredProps properties properly removedSince the
Filmclass has the@RegisterResourcedecorator, it will benefit from the automatic generation ofPropsandSecuredPropsat compile time, eliminating the need for manual declaration.src/components/progress-report/dto/team-news.dto.ts (2)
1-1: Import simplification looks goodThe removal of
keysfromts-transformer-keysandSecuredPropsfrom~/commonis appropriate as these are no longer required with the automatic property generation.
7-13: Correctly removed static Props/SecuredProps propertiesThe
ProgressReportTeamNewsclass is decorated with@RegisterResource, which will now trigger the automatic generation ofPropsandSecuredPropsproperties by the custom TypeScript transformer, making the manual declarations redundant.src/components/scripture/dto/scripture-range.dto.ts (2)
13-13: Verified removal of SecuredProps importThis import change is consistent with the PR's objective to remove the
ts-transformer-keysdependency and have the custom TS transformer automatically generate thePropsandSecuredPropsproperties at build time.
72-82: Removed static Props and SecuredProps propertiesThe static properties that were previously defined using
keysOfhave been removed as part of the transition to the custom TypeScript transformer. The@ObjectTypedecorator from@nestjs/graphqlwill trigger the automatic generation of these properties during compilation.src/components/ethno-art/dto/ethno-art.dto.ts (2)
2-2: Verified removal of importsThe import for
SecuredPropstype has been removed as it's no longer needed with the new TypeScript transformer approach.
17-25: Removed static Props and SecuredProps propertiesThe static properties previously defined using
keysOfhave been removed as part of the migration to the custom TypeScript transformer. The@RegisterResourcedecorator will trigger the automatic generation of these properties during compilation.src/components/user/unavailability/dto/unavailability.dto.ts (2)
2-2: Verified removal of importsThe import for
SecuredPropstype has been removed as it's no longer needed with the new TypeScript transformer approach.
6-21: Removed static Props and SecuredProps propertiesThe static properties previously defined using
keysOfhave been removed as part of the migration to the custom TypeScript transformer. The class is appropriately decorated with@RegisterResourcewhich will trigger the automatic generation of these properties during compilation.src/components/changeset/dto/changeset.dto.ts (2)
2-2: Verified removal of importsThe import for
SecuredPropshas been removed as expected with the introduction of the TypeScript transformer.
5-22: Removed static Props and SecuredProps propertiesThe static properties previously defined using
keysOfhave been removed as part of the migration to the custom TypeScript transformer. The class is appropriately decorated with@RegisterResourcewhich will trigger the automatic generation of these properties during compilation.src/components/progress-report/dto/community-stories.dto.ts (2)
1-1: LGTM: Import cleaned up successfullyThe import statement has been correctly simplified to only include the required
VariantOftype after removing thekeysOfandSecuredPropsimports that are no longer needed.
7-13: LGTM: Static Props and SecuredProps properties removedThe
PropsandSecuredPropsstatic properties have been successfully removed from this class. These properties will now be automatically generated by the new TypeScript transformer for classes decorated with@RegisterResource.src/components/post/dto/postable.dto.ts (2)
3-3: LGTM: Import statement cleaned up correctlyThe import statement has been properly updated to remove the no-longer-needed
keysandSecuredPropsimports while preserving the necessary types.
14-24: LGTM: Static Props and SecuredProps properties removedThe static
PropsandSecuredPropsproperties have been successfully removed from the abstractPostableclass. These properties will now be automatically generated by the new TypeScript transformer for classes decorated with@RegisterResource.src/components/project/financial-approver/dto/financial-approver.dto.ts (2)
2-2: LGTM: Import statement cleaned up correctlyThe import statement has been properly updated to remove the no-longer-needed
keysandSecuredPropsimports while preserving the necessary types.
12-17: LGTM: Static Props and SecuredProps properties removedThe static
PropsandSecuredPropsproperties have been successfully removed from theFinancialApproverclass. These properties will now be automatically generated by the new TypeScript transformer for classes decorated with@RegisterResource.src/components/post/dto/post.dto.ts (2)
3-3: LGTM: Import statement cleaned up correctlyThe import statement has been properly updated to remove the no-longer-needed
keysandSecuredPropsimports while preserving the necessary imports from the common module.
10-16: LGTM: Static Props and SecuredProps properties removedThe static
PropsandSecuredPropsproperties have been successfully removed from thePostclass. These properties will now be automatically generated by the new TypeScript transformer for classes decorated with@RegisterResourceand that implementResource.src/components/progress-report/dto/highlights.dto.ts (1)
1-1: LGTM! Removal of manual Props/SecuredProps declarations.The removal of the explicit imports and static
Props/SecuredPropsdeclarations aligns with the PR's objective to automatically generate these properties at compile time for any class in a.dto.tsfile with the@RegisterResourcedecorator.src/components/project-change-request/dto/project-change-request.dto.ts (1)
2-2: LGTM! Removal of manual Props/SecuredProps declarations.The removal of the explicit import for
keysand modification of the imports from~/commonaligns with the PR's objective. The staticPropsandSecuredPropsproperties will now be automatically generated at compile time by the custom transformer for this class, which is properly decorated with@RegisterResource()and@ObjectType().src/components/comments/dto/comment.dto.ts (1)
3-3: LGTM! Removal of manual Props/SecuredProps declarations.The consolidation of imports and removal of explicit dependencies on
keysfromts-transformer-keysis consistent with the PR's objective. The class properly meets the criteria for automatic generation ofPropsandSecuredPropsproperties, being in a.dto.tsfile and decorated with both@RegisterResourceand@ObjectType.src/core/resources/plugin/index.js (1)
1-25: LGTM! Well-structured TypeScript transformer plugin.The implementation follows standard TypeScript transformer plugin patterns with proper exports for both default usage and integration with ts-patch and Nest CLI. Using JavaScript with JSDoc type annotations is appropriate for transformer plugins to avoid circular dependencies during compilation.
Could you clarify the purpose of re-exporting
ResourceVisitorasReadonlyVisitoron line 17? This suggests there might be plans for external use beyond this plugin.src/components/project/dto/project.dto.ts (1)
223-223: LGTM – relies on new transformer
TranslationProjectnow delegates all metadata generation to the new transformer.
No issues spotted with the minimal class body.
Any class within a
.dto.tsfile that is decorated with one of the@nestjs/graphqldecorators or ourRegisterResourcedecorator will automatically defineProps&SecuredPropsfor use at runtime.TS isn't statically aware of this, but it can be accessed via
EnhancedResource:These are strictly typed Sets.
This needed #3422, #3421, #3420, #3416, #3415
Currently the plugin updates the transpired JS directly.
I've stubbed the "readonly" code path which is used with SWC.
I'm not entirely convinced SWC will help overall - if TS compiler is still needed to generate this metadata, and that is not cached or something, then it is probably worse than this current setup.