@tus/s3-store: add s3Client option

[mitjap]

With this change, S3 Client can be injected instead of being created internally, which enables mocking and improves testability in general.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4b84f80

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@tus/s3-store	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

.changeset/shy-buckets-knock.md (1)

5-5: Fix grammar in the description.

Add the missing article "the" before "user".

-Add `s3Client` option. This allows user to provide existing S3 client to datastore.
+Add `s3Client` option. This allows the user to provide existing S3 client to datastore.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: --- "@tus/s3-store": minor --- Add s3Client option. This allows user to pr...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

packages/s3-store/test/index.ts (1)

298-315: Enhance test coverage for custom S3 client.

While the test verifies basic functionality, consider enhancing it to:

1. Verify other S3 operations (e.g., createMultipartUpload, completeMultipartUpload)
2. Assert that a new client is not created when custom client is provided
3. Add cleanup in an afterEach block to restore mocks

 it('should use custom S3 client when specified', async () => {
   const client = new S3(s3ClientConfig);
   const store = new S3Store({
     s3Client: client,
     s3ClientConfig,
   });

   const clientMock = sinon.mock(client);
+  clientMock.expects("createMultipartUpload").once();
   clientMock.expects("putObject").once();
+  clientMock.expects("completeMultipartUpload").once();

   await store.create(new Upload({
     id: shared.testId('test-upload'),
     size: 10 * 1024 * 1024,
     offset: 0,
   }))

   clientMock.verify();
+  clientMock.restore();
 })

packages/s3-store/README.md (1)

94-97: Add example for s3Client usage.

The documentation clearly explains the option, but would benefit from a concrete example showing how to use it, especially in a testing context since that's a primary use case.

Add an example like:

const { S3 } = require('@aws-sdk/client-s3');
const { S3Store } = require('@tus/s3-store');

// Create a custom S3 client (e.g., for testing)
const customClient = new S3({
  credentials: {
    accessKeyId: 'test',
    secretAccessKey: 'test',
  },
  region: 'us-east-1',
});

// Use the custom client with S3Store
const s3Store = new S3Store({
  s3Client: customClient,
  s3ClientConfig: {
    bucket: 'my-test-bucket',
  },
});

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f47f371 and 4b84f80.

📒 Files selected for processing (4)

• .changeset/shy-buckets-knock.md (1 hunks)
• packages/s3-store/README.md (1 hunks)
• packages/s3-store/src/index.ts (2 hunks)
• packages/s3-store/test/index.ts (2 hunks)

🧰 Additional context used

🪛 LanguageTool

.changeset/shy-buckets-knock.md

[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: --- "@tus/s3-store": minor --- Add s3Client option. This allows user to pr...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

🔇 Additional comments (2)

.changeset/shy-buckets-knock.md (1)

2-2: LGTM: Version bump is appropriate.

The "minor" version bump is correct as adding a new optional configuration property (s3Client) is a backward-compatible feature addition.

packages/s3-store/src/index.ts (1)

50-50: LGTM! Clean implementation of the s3Client option.

The implementation correctly:

• Makes s3Client optional in the Options type
• Uses nullish coalescing for backward compatibility
• Maintains the requirement for bucket configuration

Also applies to: 131-131

[Murderlon]

I'm not a fan of this change in its current form. I think it's confusing to have bucket, s3ClientConfig, and s3Client. Furthermore, people will get warnings that they import the s3 client without having it installed directly.

I'm okay with one of the following:

1. Make a breaking change that moves @aws-sdk/client-s3 to peer dependencies and remove s3ClientConfig option. Only keep the new s3Client.
2. Leave it as is. No one asked for it (yet) and I haven't run into a scenario where this was blocking to mock a test. All s3 client operations are wrapped in a class method so you can just mock the class method instead.

[mitjap]

I'm fine with leaving it as is. It's a good suggestion to just mock a wrapped class method :)