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

Upgrade to Google Ads API v17.1 #122

Closed
Raymondo97 opened this issue Aug 8, 2024 · 5 comments
Closed

Upgrade to Google Ads API v17.1 #122

Raymondo97 opened this issue Aug 8, 2024 · 5 comments

Comments

@Raymondo97
Copy link

Google Ads API updated their version to 17.1 as of yesterday (Aug 7th, 2024). I was hoping to update this package, along with the Ads API package (https://github.com/Opteo/google-ads-api).

In order to do so, I see that the instructions give direction to pull the latest changes from gax-nodejs (https://github.com/Opteo/gax-nodejs). I also noticed that this project is behind by 80 commits from the Google Gax package. I tried to create a pull request for that package. But I can't, since I'm not a contributor.

Would we be able to get started on this update?

@Raymondo97
Copy link
Author

Raymondo97 commented Aug 9, 2024

Also, I found a clue as to what may be causing the case issues that are mentioned in this comment:

# horrible hack but no easy way to resolve this
# requires preventing request parameters being compiled to snakeCase
# in this dependency: https://github.com/googleapis/gapic-generator-typescript/blob/master/templates/typescript_gapic/src/%24version/%24service_client.ts.njk
# i will personally buy a beer to whoever can solve this, good luck.

The final comment of an issue reported in the gapic-generator-typescript package give some insight. googleapis/gapic-generator-typescript#961. It references this change: Opteo/gax-nodejs@a1c226a

I don't think I understand enough of the code to definitively say whether this is the answer. But it definitely seems like a possibility. Let me know what you think.

@Raymondo97
Copy link
Author

Raymondo97 commented Aug 13, 2024

I found out something else concerning the casing. There is an option in gapic-tools that allows for keeping the generated protos in snake_case. https://github.com/googleapis/gax-nodejs/blob/d74fa409897468af5dba78ef60bb5d4c9defacb6/tools/src/compileProtos.ts#L409 The only issue that I've seen with it, is that the tests are designed to check for camelCase.

In the attached links, others have asked about the snake_case & camelCase issues. From what I have researched, Google developers almost always say that camelCase is the standard casing for JavaScript. So they've purposefully coded any nodejs packages and builds to convert to camelCase. So I wonder if we may want to add support for each case, depending on whether the --keep-case param is set.

https://github.com/googleapis/gax-nodejs/blob/e6669acbf366534c74b8ad99691174288f73e60e/test/unit/transcoding.ts#L157

googleapis/gapic-generator-typescript#1126

googleapis/gax-nodejs#1312

@Raymondo97
Copy link
Author

Looks l like we could use Google's googleapis/gax-nodejs package directly. It should work because of this pull request: googleapis/gax-nodejs#1561

@evil-shrike
Copy link

It's three months have passed since v17 release. Please upgrade to v17 or (better) to v18 which is around the corner.

@avermeil
Copy link
Member

avermeil commented Sep 9, 2024

Done :)

@Raymondo97, you're right that we can use the original gax-nodejs package instead of our fork thanks to their addition of the keep-case and force-number arguments, which they pass trough to pbjs. I've updated this lib to reflect this.

@avermeil avermeil closed this as completed Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants