googleapis / nodejs-storage

Node.js client for Google Cloud Storage: unified object storage for developers and enterprises, from live data serving to data analytics/ML to data archiving.
https://cloud.google.com/storage/
Apache License 2.0
900 stars 369 forks source link

Dynamic import results in esm #2555

Open prescottprue opened 2 days ago

prescottprue commented 2 days ago

Please make sure you have searched for information in the following guides.

A screenshot that you have tested with "Try this API".

N/A - types issue

Link to the code that reproduces this issue. A link to a public Github Repository or gist with a minimal reproduction.

https://gist.github.com/prescottprue/b77e59c85941d2e93766e9055819da99

A step-by-step description of how to reproduce the issue, based on the linked reproduction.

  1. Import types via import type { Storage, File } from '@google-cloud/storage (typescript by default uses cjs types unless you are doing full ESM
  2. Dynamically import lib functionality via:
    const { Storage } = await import('@google-cloud/storage')`
    const storage = new Storage() // <- is ESM types, not CSJ
    const file = storage.bucket(bucket).file(filePath) // not the same as File type

A clear and concise description of what the bug is, and what you expected to happen.

CJS types are pulled in when import type if repo is not ESM, but using dynamic import causes ESM types to be pulled in. Since the types are not the same there is an error saying:

Type 'import(".../node_modules/@google-cloud/storage/build/esm/src/file", { with: { "resolution-mode": "import" } }).File' is not assignable to type 'import(".../identity-service/node_modules/@google-cloud/storage/build/cjs/src/file").File'.
  Property '#private' in type 'File' refers to a different member that cannot be accessed from within type 'File'.

I've also tried the resolution-mode on both the import and the type import but that didn't work (regardless of import/reuqire setting):

import type { Storage, File } from '@google-cloud/storage' with { "resolution-mode": "import" };
// or
import type { Storage, File } from '@google-cloud/storage' with { "resolution-mode": "require" };
const { Storage } = await import('@google-cloud/storage', { with: { 'resolution-mode': 'import' } })
// or
const { Storage } = await import('@google-cloud/storage', { with: { 'resolution-mode': 'require' } })

A clear and concise description WHY you expect this behavior, i.e., was it a recent change, there is documentation that points to this behavior, etc. **

It is expected that the types match between cjs and esm as they do with @google-cloud/pubsub: https://github.com/googleapis/nodejs-pubsub/blob/main/package.json. With PubSub lib dynamic import is no issue

ddelgrosso1 commented 1 day ago

Thanks for reporting this @prescottprue. I'll take a look if there is anything we can do to get the correct types under these conditions. I'm a bit curious as to the use case for dynamically importing the library when there is a CJS version available?

prescottprue commented 11 hours ago

@ddelgrosso1 In serverless environments environments such as Cloud Run it is helpful to only load dependencies once needed instead of globally to improve cold start times as called out within the Cloud Run Tips Docs for optimizing performance. We have also had this reenforced to us by Google support engineers when attempting to bring down our cold start latency

In theory it could be dynamically required, but we prefer import since it allows for static analysis (also keeps things consistent)