Skip to content
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

Dynamic import results in esm #2555

Open
7 tasks done
prescottprue opened this issue Nov 14, 2024 · 2 comments
Open
7 tasks done

Dynamic import results in esm #2555

prescottprue opened this issue Nov 14, 2024 · 2 comments
Labels
api: storage Issues related to the googleapis/nodejs-storage API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@prescottprue
Copy link

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

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Nov 14, 2024
@ddelgrosso1
Copy link
Contributor

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?

@ddelgrosso1 ddelgrosso1 added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Nov 14, 2024
@prescottprue
Copy link
Author

prescottprue commented Nov 15, 2024

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants