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

fix: do not use #private #1454

Merged
merged 1 commit into from
Aug 31, 2022
Merged

fix: do not use #private #1454

merged 1 commit into from
Aug 31, 2022

Conversation

alexander-fenster
Copy link
Contributor

Adding a # private method is technically a breaking change. Our packing tests across all client libraries are now failing with

node_modules/google-auth-library/build/src/auth/googleauth.d.ts:67:5 - error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.

67     #private;
       ~~~~~~~~

(in CI, this looks like a pack-n-play TypeScript test failure as a part of system tests)

Let's not be that modern and use the regular private method instead.

@alexander-fenster alexander-fenster requested review from a team as code owners August 31, 2022 23:02
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Aug 31, 2022
@danielbankhead danielbankhead merged commit 6c30274 into main Aug 31, 2022
@danielbankhead danielbankhead deleted the no-private branch August 31, 2022 23:14
gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 31, 2022
🤖 I have created a release *beep* *boop*
---


## [8.5.1](v8.5.0...v8.5.1) (2022-08-31)


### Bug Fixes

* do not use #private ([#1454](#1454)) ([6c30274](6c30274))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@ruyadorno
Copy link

Oh that's interesting, private class fields have been on Node.js since v12. I believe this is more of a matter of reconfiguring our TypeScript compiler to a little more modern level of ECMAScript support rather than being an unsupported feature.

That said, there's maybe an argument to be made on what is the more JS-idiommatic way of expressing the intent of a private method/property vs TS-idiommatic and I'm not sure where the collective consensus within the larger JavaScript ecosystem sits on that.

Anyways just wanted to highlight that it might be possible to support #private notations and that it might be more familiar to a lot of JS devs and hear your thoughts on it @alexander-fenster (I might be missing more context here, feel free to correct me if I'm making any wrong assumption) 😊

@danielbankhead
Copy link
Contributor

@ruyadorno no problem, we just need to update gts & TypeScript in https://github.com/google/pack-n-play (the repo needs some love).

@DesAWSume
Copy link

a year after, this issue bothers me.
`node_modules/google-auth-library/build/src/util.d.ts:134:5 - error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.

134 #private;
~~~~~~~~

Found 1 error.`

@danielbankhead
Copy link
Contributor

@DesAWSume the error message explains what to do; simply target es2015+ in TypeScript:

@DesAWSume
Copy link

Oh~. 2nd look. I have a tsconfig under src folder, which my yarn build command will target to it.
My bad. It fixed for me~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants