refactor(storage): dynamically import mime and p-limit with module caching - #9230
refactor(storage): dynamically import mime and p-limit with module caching#9230thiyaguk09 wants to merge 7 commits into
Conversation
… improve startup performance
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces lazy loading for the mime and p-limit dependencies to improve startup performance. Direct imports of these modules have been replaced with helper functions getMime and getPLimit in util.ts which dynamically import and cache the modules. The Bucket, File, and TransferManager classes have been updated to asynchronously retrieve these dependencies when needed, and the corresponding test suites have been updated to mock these new helpers. I have no feedback to provide as there are no review comments.
| } else { | ||
| this.startResumableUpload_(fileWriteStream, options); | ||
| } | ||
| writeStream.once('writing', async () => { |
There was a problem hiding this comment.
gzip handling is split between the synchronous setup (gzip === true uses push) and the async 'writing' callback (gzip === 'auto' uses unshift).
Could we consolidate MIME detection, gzip evaluation, and transformStreams construction inside the 'writing' handler?
Additionally, if options.contentType or options.metadata.contentType is already set (and not 'auto'), we can skip calling getMime() entirely.
| * | ||
| * @internal | ||
| */ | ||
| export async function getPLimit(): Promise<typeof import('p-limit')> { |
There was a problem hiding this comment.
This method returns the callable limiter function directly (modObj.default || modObj), but its return type is declared as the module namespace Promise<typeof import('p-limit')>.
To align the type with runtime behavior and prevent typing issues with pLimit(concurrency), consider refining the return type.
And similarly ensure getMime() reflects the resolved MIME interface with .getType().
There was a problem hiding this comment.
Could we add dedicated unit tests under test/util.ts for getMime() and getPLimit() covering:
- Promise caching
- Error recovery
| }, | ||
| './util.js': { | ||
| ...require('../src/util.js'), | ||
| getPLimit: async () => fakePLimit, |
There was a problem hiding this comment.
Is this line required? src/bucket.ts imports getPLimit via ./util.js
2 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #7350