docs(storage): add gaxios and node 18 migration guide#8340
Conversation
…sport (googleapis#8283) - Remove Service.ts and common.ts files from handwritten/storage - Migrate remaining functionality to StorageTransport - chore(ci): upgrade conformance tests to Node 18
…ios-migration-guide
…ios-migration-guide
There was a problem hiding this comment.
Code Review
This pull request refactors the library to version 8, bumping the minimum Node.js requirement to 18 and replacing the legacy request stack with a new StorageTransport layer based on gaxios and native Web APIs. The review identifies several critical issues in the new transport implementation, including a module-level global variable causing race conditions for project IDs, incorrect URL resolution logic, and broken interceptor support. Further feedback points out that per-object interceptors are no longer being applied, identifies fragile link extraction logic in GitHub scripts, and notes potential bugs related to unawaited async calls and synchronous project ID usage before it is fetched.
I am having trouble creating individual review comments. Click here to see my feedback.
handwritten/storage/src/storage-transport.ts (90)
The use of a module-level global variable projectId to store state across requests is a critical bug. In a concurrent environment, multiple Storage instances or concurrent requests will overwrite this shared variable, leading to race conditions where a request for one project might use the ID of another. This state must be encapsulated within the StorageTransport instance.
handwritten/storage/src/storage-transport.ts (192)
The URL construction logic uses string concatenation (${this.baseUrl}${pathUri}) instead of the native URL resolution rules. As noted in MIGRATION.md, a leading slash in pathUri should resolve relative to the host root, stripping segments like /storage/v1 from the baseUrl. The current implementation will incorrectly produce URLs with duplicated segments (e.g., .../storage/v1/storage/v1/...) because many paths in the library now include the full API prefix.
url = new URL(pathUri, this.baseUrl);
handwritten/storage/conformance-test/conformanceCommon.ts (63)
This TODO effectively disables the methodMap initialization by using an empty object. This will cause conformance tests to skip all scenarios or fail silently, as the mapping between JSON test methods and library methods is lost.
Object.entries(jsonToNodeApiMapping),
handwritten/storage/src/nodejs-common/service-object.ts (448-464)
The new implementation of getMetadata (and similarly setMetadata and delete) fails to pass this.interceptors to this.storageTransport.makeRequest. In previous versions, ServiceObject would merge parent and local interceptors. This change breaks the functionality of per-object interceptors (e.g., interceptors added specifically to a File or Bucket instance).
handwritten/storage/src/storage-transport.ts (134-139)
There are two issues here: 1) Clearing and adding interceptors on a shared this.gaxiosInstance is not thread-safe for concurrent requests. 2) The actual request is performed via this.authClient.request, which uses its own internal transporter and ignores the interceptors added to this.gaxiosInstance. This effectively breaks all interceptor support in the library.
handwritten/storage/.github/scripts/close-invalid-link.cjs (45)
The logic for extracting the reproduction link is extremely fragile as it relies on a hardcoded line index (18). If a user adds extra lines or if the issue template is modified, this script will fail to find the link or pick up incorrect data. It is better to search the entire issue body for a GitHub/Gist URL following the expected section header.
handwritten/storage/src/bucket.ts (2081)
If this.storage.projectId is not provided in the constructor and has not yet been fetched via an internal request (which triggers the async getProjectId in StorageTransport), this topic string will be incorrectly constructed as projects/undefined/topics/.... Since this is a synchronous assignment, it should account for the possibility of the project ID being unavailable.
handwritten/storage/src/file.ts (1478)
The CopyCallback expects three arguments: (err, file, apiResponse). Calling it with only the error argument may cause issues for consumers expecting the full signature, especially if they are using promisified versions of the library that rely on specific argument positions.
.catch(err => callback!(err, null, null));
handwritten/storage/src/resumable-upload.ts (1362)
The call to attemptDelayedRetry is not awaited. Since attemptDelayedRetry is an async function that can call startUploading (also async), any rejection within that chain will result in an unhandled promise rejection because the caller (onResponse) has already returned.
await this.attemptDelayedRetry(resp);
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 #<issue_number_goes_here> 🦕