-
Notifications
You must be signed in to change notification settings - Fork 227
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
build!: convert to typescript #923
Conversation
Codecov Report
@@ Coverage Diff @@
## master #923 +/- ##
=======================================
Coverage 94.06% 94.06%
=======================================
Files 23 23
Lines 10662 10662
Branches 503 503
=======================================
Hits 10029 10029
Misses 630 630
Partials 3 3 Continue to review full report at Codecov.
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@@ -50,7 +50,7 @@ function main( | |||
}); | |||
|
|||
async function publishWithRetrySettings() { | |||
const formattedTopic = publisherClient.topicPath(projectId, topicName); | |||
const formattedTopic = publisherClient.projectTopicPath(projectId, topicName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried this breaking change will get lost in the shuffle for folks. Can we make this super clear in the breaking changes section of the change log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. But not very clear here, should I update change log in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcoe what's the recommended way to do this? I've done longer form "before / after" descriptions for breaking changes that were meaningful in the past. Is the process here to manually update the CHANGELOG in a followup PR?
clients = ['publisher', 'subscriber'] | ||
for client_name in clients: | ||
client_file = f'src/v1/{client_name}_client.ts' | ||
s.replace(client_file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hey, this is great!
r"https://\1)") | ||
# fix tslint issue due to mismatch gts version with gapic-generator-typescript | ||
# it should be removed once pubsub upgrade gts 2.0.0 | ||
s.replace(client_file, '\/\/ eslint\-disable\-next\-line\ \@typescript\-eslint\/no\-explicit\-any', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO
above says we need to upgrade to [email protected]
. That happens here right? Can we get rid of this synth change?
tsconfig.json
Outdated
"outDir": "build", | ||
"resolveJsonModule": true, | ||
"lib": [ | ||
"es2016", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still says 2016 :)
@@ -22,7 +22,7 @@ import {promisifyAll} from '@google-cloud/promisify'; | |||
import arrify = require('arrify'); | |||
import {CallOptions} from 'google-gax'; | |||
|
|||
import {google} from '../proto/iam'; | |||
import {google} from '../protos/protos'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this iam.ts
do? some exported type like SetPolicyResponse
can be found in protos.google.iam.v1.IAMPolicy
. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! Thank you Summer!
@@ -50,7 +50,7 @@ function main( | |||
}); | |||
|
|||
async function publishWithRetrySettings() { | |||
const formattedTopic = publisherClient.topicPath(projectId, topicName); | |||
const formattedTopic = publisherClient.projectTopicPath(projectId, topicName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcoe what's the recommended way to do this? I've done longer form "before / after" descriptions for breaking changes that were meaningful in the past. Is the process here to manually update the CHANGELOG in a followup PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On re-review, this looks pretty nice! I would take another look down the remaining comments, but even as it is, it's fine. We can always tweak it a bit before 2.0.0 if something comes up.
"prettier": "^1.18.2", | ||
"proxyquire": "^2.0.0", | ||
"sinon": "^9.0.0", | ||
"source-map-support": "^0.5.9", | ||
"ts-loader": "^6.2.1", | ||
"typescript": "3.6.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use "typescript": "^3.8.3"
as we do for other libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussion with @feywind, upgrade typescript ^3.8.3
will happen when pubsub is ready for deprecate Node 8 and upgrade [email protected].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
👏 👏 👏 |
* docs: fix families typo * docs: fix initial typo
BREAKING CHANGE:
topicPath
has been changed toprojectTopicPath
Some synthtool hack will be removed once pubsub upgrade to
gts:2.0.0