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

Question: why isn't protos.json loaded with a require call in Node environment? #1493

Closed
hmaurer opened this issue May 4, 2021 · 4 comments
Assignees
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@hmaurer
Copy link

hmaurer commented May 4, 2021

Hello!

During an attempt to bundle (via esbuild) a Node application using the firebase-admin SDK I noticed that protos.json is loaded using a require call in a browser environment, but loaded differently in a Node environment, c.f.

require('../../protos/protos.json')

While I acknowledge that bundling Node applications may not be a use-case you want to support, I was wondering why you made the explicit choice of not using require in a Node environment? My suggestion would be to use require in all environments, like so:

this._protos = this._gaxGrpc.loadProto(require('../../protos/protos.json'));

It'd appear that I am not the only one who ran into this issue:

Bundling back-end applications has some benefits (in my case, generate the smallest possible package for services deployed to Cloud Run / Cloud Functions), so if making this small change would enable it it'd be wonderful!

Thanks for your work.


Related threads:

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label May 4, 2021
@hmaurer
Copy link
Author

hmaurer commented May 4, 2021

Update: it looks like my suggestion has been implemented a few days ago: googleapis/gapic-generator-typescript#861! There's only one question left then: when will this come to this package? I'd be happy to submit a pull request bumping google-gax to 2.12.0 and switching to require, as suggested by googleapis/gapic-generator-typescript#861 (comment).

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label May 8, 2021
@alexander-fenster
Copy link
Contributor

@hmaurer It came in today :) #1500 is where it happened.

@hmaurer
Copy link
Author

hmaurer commented May 8, 2021

@alexander-fenster Wonderful, thanks! <3

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label May 10, 2021
@crwilcox crwilcox added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels May 11, 2021
@yoshi-automation yoshi-automation removed 🚨 This issue needs some love. triage me I really want to be triaged. labels May 12, 2021
@alexander-fenster
Copy link
Contributor

Fixed by #1500.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. 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

4 participants