-
Notifications
You must be signed in to change notification settings - Fork 0
Added new rule 'require-aws-bare-bones' #137
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new ESLint rule 'require-aws-bare-bones' that enforces the use of bare-bones AWS SDK v3 client patterns (Client + Command) instead of aggregated clients to improve tree-shaking and reduce bundle size.
- Added a new ESLint rule that detects and prevents imports of aggregated AWS SDK v3 clients
- Integrated the rule into the plugin's configuration and rule exports
- Created comprehensive test coverage for valid and invalid usage patterns
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/require-aws-bare-bones.ts | Implements the core rule logic to detect aggregated AWS SDK client imports |
| src/require-aws-bare-bones.spec.ts | Provides comprehensive test cases for both valid and invalid usage patterns |
| src/index.ts | Integrates the new rule into the plugin's rule registry and configurations |
| package.json | Updates version number to reflect the new rule addition |
| docs/rules/require-aws-bare-bones.md | Documents the rule with examples of correct and incorrect usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
docs/rules/require-aws-bare-bones.md
Outdated
| import { DynamoDB } from '@aws-sdk/client-dynamodb'; | ||
|
|
||
| const ddb = new DynamoDB({}); | ||
| await ddb.putItem({}); |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation shows incorrect 'Pass' example. This code imports the aggregated DynamoDB client which should fail according to the rule. It should be 'import { DynamoDBClient, PutItemCommand }' instead.
| import { DynamoDB } from '@aws-sdk/client-dynamodb'; | |
| const ddb = new DynamoDB({}); | |
| await ddb.putItem({}); | |
| import { DynamoDBClient, PutItemCommand } from '@aws-sdk/client-dynamodb'; | |
| const ddb = new DynamoDBClient({}); | |
| await ddb.send(new PutItemCommand({})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/require-aws-bare-bones.ts
Outdated
| // require-aws-bare-bones.ts | ||
|
|
||
| import { AST_NODE_TYPES, ESLintUtils, TSESTree } from '@typescript-eslint/utils'; | ||
| import getDocumentationUrl from './get-documentation-url.ts'; |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement includes a .ts extension which is not typically needed in TypeScript imports and may cause issues with module resolution in some environments.
| import getDocumentationUrl from './get-documentation-url.ts'; | |
| import getDocumentationUrl from './get-documentation-url'; |
src/require-aws-bare-bones.spec.ts
Outdated
| // require-aws-bare-bones.spec.ts | ||
|
|
||
| import rule, { MESSAGE_ID_AGGREGATED_CLIENT, ruleId } from './require-aws-bare-bones.ts'; | ||
| import createTester from './ts-tester.test.ts'; |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statements include .ts extensions which are not typically needed in TypeScript imports and may cause issues with module resolution in some environments.
| import createTester from './ts-tester.test.ts'; | |
| import createTester from './ts-tester.test'; |
src/index.ts
Outdated
| ruleId as requireServiceCallResponseDeclarationRuleId, | ||
| } from './require-service-call-response-declaration.ts'; | ||
| import requireAwsConfig, { ruleId as requireAwsConfigRuleId } from './require-aws-config.ts'; | ||
| import requireAWSBareBones, { ruleId as requireAWSBareBonesRuleId } from './require-aws-bare-bones.ts'; |
Copilot
AI
Oct 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement includes a .ts extension which is not typically needed in TypeScript imports and may cause issues with module resolution in some environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always use .ts extension targeting ESM output in TypeScript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Tested with internal services |
carlansley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no direct overlap: one is about import patterns, the other about configuration usage. |
.github/workflows/publish-beta.yml
Outdated
| run: npm install -g npm@11.4.2 | ||
| - name: Install Dependencies | ||
| run: npm ci --ignore-scripts | ||
| run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason "--ignore-scripts" is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
publish beta fails Error: The package "@esbuild/linux-x64" could not be found, and is needed by esbuild. npm ci is allowing this binary to be installed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were able to keep ignore-scripts in other repos though, are you sure that we have to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rolled back this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/aws/require-aws-bare-bones.ts
Outdated
| }, | ||
| messages: { | ||
| [MESSAGE_ID_AGGREGATED_CLIENT]: | ||
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{clientName}}Client + Command) instead.', |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message assumes the bare-bones client name is always {{clientName}}Client, which is incorrect for services like StepFunctions (actual client is SFNClient). Rephrase to a generic instruction (e.g., 'Use the service-specific *Client class plus Command classes') or add logic to derive/display the correct client name for known exceptions.
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{clientName}}Client + Command) instead.', | |
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use the service-specific *Client class plus Command classes instead.', |
src/aws/require-aws-bare-bones.ts
Outdated
| const isException = specifier.local.name.endsWith('Exception'); | ||
|
|
||
| if ( | ||
| specifier.type === AST_NODE_TYPES.ImportSpecifier && | ||
| !isTypeImport && | ||
| !isException && |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isException guard is redundant because names ending with 'Exception' already satisfy BARE_BONES_SUFFIXES and make isAggregatedClient return false. You can remove isException and its check to simplify the condition.
| const isException = specifier.local.name.endsWith('Exception'); | |
| if ( | |
| specifier.type === AST_NODE_TYPES.ImportSpecifier && | |
| !isTypeImport && | |
| !isException && | |
| if ( | |
| specifier.type === AST_NODE_TYPES.ImportSpecifier && | |
| !isTypeImport && |
docs/rules/require-aws-bare-bones.md
Outdated
| # Enforce Bare-Bones AWS SDK v3 Client Usage | ||
|
|
||
| Ensure that only bare-bones AWS SDK v3 clients and commands are imported and used. Importing and using aggregated clients (e.g., `S3`, `DynamoDB`) is disallowed to promote modularization and enable better tree-shaking for reduced bundle size. |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding an example for a service where the bare-bones client name differs from the aggregated name (e.g., aggregated StepFunctions vs SFNClient) to clarify that the pattern is not always AggregatedName + Client.
src/aws/require-aws-bare-bones.ts
Outdated
|
|
||
| function isAwsSdkClientModule(importDeclaration: TSESTree.ImportDeclaration): boolean { | ||
| return ( | ||
| typeof importDeclaration.source.value === 'string' && importDeclaration.source.value.startsWith('@aws-sdk/client-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we handle document related stuff from "@aws-sdk/lib-*" as well? e.g. DynamoDBDocument from '@aws-sdk/lib-dynamodb'
src/aws/require-aws-bare-bones.ts
Outdated
|
|
||
| for (const specifier of node.specifiers) { | ||
| const isTypeImport = specifier.type === AST_NODE_TYPES.ImportSpecifier && specifier.importKind === 'type'; | ||
| const isException = specifier.local.name.endsWith('Exception'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it duplicated with the above regexp?
.github/workflows/publish-beta.yml
Outdated
| run: npm install -g npm@11.4.2 | ||
| - name: Install Dependencies | ||
| run: npm ci --ignore-scripts | ||
| run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were able to keep ignore-scripts in other repos though, are you sure that we have to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| [MESSAGE_ID_AGGREGATED_CLIENT]: | ||
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{clientName}}Client/ Lib plus Command) instead.', | ||
| }, |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message suggests constructing {{clientName}}Client, which is incorrect for aggregated lib imports like DynamoDBDocument (correct base is DynamoDBClient). Also the fragment 'Client/ Lib' has an extra space and slash formatting issue. Recommend computing a baseClientName (derived from importSource) and changing the message to: 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{baseClientName}}Client plus Command) instead.' Add baseClientName to data when reporting and remove the malformed 'Client/ Lib' formatting.
| code: `import { DynamoDBDocumentClient, PutCommand } from '@aws-sdk/lib-dynamodb'; | ||
| import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; | ||
| import awsConfig from '@checkdigit/aws-config'; | ||
| const dynamo = awsConfig(DynamoDBClient, { qualifier, environment }); | ||
| const dynamoDocument = DynamoDBDocument.from(dynamo); | ||
| await dynamoDocument.send(new PutCommand({ TableName: 'foo', Item: { id: 1 } }));`, |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet uses DynamoDBDocument.from but never imports DynamoDBDocument; instead it imports DynamoDBDocumentClient which is unused. This would produce an undefined reference. Replace DynamoDBDocumentClient with DynamoDBDocument in the import or change usage to DynamoDBDocumentClient.from if that API is intended.
src/aws/require-aws-bare-bones.ts
Outdated
| 'Disallow importing aggregated AWS SDK v3 clients. Use bare-bones pattern (Client/Lib plus Command) for better tree-shaking.', | ||
| }, | ||
| messages: { | ||
| [MESSAGE_ID_AGGREGATED_CLIENT]: | ||
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{clientName}}Client/ Lib plus Command) instead.', |
Copilot
AI
Oct 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Description uses 'Client/Lib plus Command'; consider clarifying to 'Client (e.g., S3Client) plus specific Command(s)' to improve precision and remove the slash ambiguity.
| 'Disallow importing aggregated AWS SDK v3 clients. Use bare-bones pattern (Client/Lib plus Command) for better tree-shaking.', | |
| }, | |
| messages: { | |
| [MESSAGE_ID_AGGREGATED_CLIENT]: | |
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{clientName}}Client/ Lib plus Command) instead.', | |
| 'Disallow importing aggregated AWS SDK v3 clients. Use bare-bones pattern (Client (e.g., S3Client) plus specific Command(s)) for better tree-shaking.', | |
| }, | |
| messages: { | |
| [MESSAGE_ID_AGGREGATED_CLIENT]: | |
| 'Do not import aggregated AWS SDK v3 client "{{clientName}}". Use bare-bones pattern ({{clientName}}Client (e.g., S3Client) plus specific Command(s)) instead.', |
le-cong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a minor comment
| return !endsWithAnySuffix(name, BARE_BONES_SUFFIXES); | ||
| } | ||
| if (importSource.startsWith(AWS_SDK_LIB)) { | ||
| const pkg = importSource.replace(AWS_SDK_LIB, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should BARE_BONES_SUFFIXES be checked for lib-* stuff as well?
btw the naming pattern matching for lib stuff feels a bit tricky, but hopefully it's good enough to cover what we normally use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya pretty much it should be good for the ones we use, else we can extend to have other usages as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| code: `import { DynamoDBDocumentClient, PutCommand } from '@aws-sdk/lib-dynamodb'; | ||
| import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; | ||
| import awsConfig from '@checkdigit/aws-config'; | ||
| const dynamo = awsConfig(DynamoDBClient, { qualifier, environment }); | ||
| const dynamoDocument = DynamoDBDocument.from(dynamo); | ||
| await dynamoDocument.send(new PutCommand({ TableName: 'foo', Item: { id: 1 } }));`, |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses DynamoDBDocument without importing it (only DynamoDBDocumentClient is imported); update import to include DynamoDBDocument or change usage to DynamoDBDocumentClient.from to avoid an unresolved identifier.
le-cong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hate to agree with some unsolved AI comments, otherwise lgtm
some of them are outdated and others are mostly pointing to spec file |
|
Beta Published - Install Command: |
|
❌ PR review status - has 1 reviewer outstanding |
Closes #135