-
Notifications
You must be signed in to change notification settings - Fork 99
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(ts): do not require storage/pubsub types, add install test #430
Conversation
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
======================================
Coverage 90.6% 90.6%
======================================
Files 14 14
Lines 628 628
Branches 32 32
======================================
Hits 569 569
Misses 41 41
Partials 18 18
Continue to review full report at Codecov.
|
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
destination: Bucket|Dataset|Topic|string; | ||
// destination: Bucket|Dataset|Topic|string; | ||
// tslint:disable-next-line no-any | ||
destination: 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.
aha, this is the same thing I did in my local testing; glad that I'm figuring these things out.
* application. | ||
*/ | ||
it('should be able to use the d.ts', async () => { | ||
await execa('npm', ['pack', '--unsafe-perm']); |
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 like this approach to integration testing the TypeScript module, I wonder if this could be be abstracted into a module.
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.
@JustinBeckwith I see that you chose not to use post-install-check. I am guessing that there are things in that module that you don't like. Can you work with @DominicKramer and provide feedback to make sure we are serving your needs and making that module better for everyone ❤️.
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.
Great call out. Feedback left!
https://github.com/google/post-install-check/issues
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, I wasn't trying to be a , I didn't realize this was essentially what post-install-check
does.
It would be nice to eventually consolidate on a module like post-install-check
... I honestly don't love the name post-install-check
if what it does is check that TypeScript has the appropriate dependencies. I read post-install-check
and assumed it was something more abstract (a generic way of checking your packages post-install scripts?). I'd advocate a name like check-installed-types
?
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.
Yep, it is not the best name. We can rename it. How about something like ts-pack-n-play
?
/cc @googleapis/node-team FYI
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.
FWIW - I love that name. @ofrobots wanna open an issue over there to consider the rename?
@@ -66,7 +66,9 @@ export type DeleteResponse = google.protobuf.Empty; | |||
export type LogSink = google_config.logging.v2.ILogSink; | |||
|
|||
export interface CreateSinkRequest { | |||
destination: Bucket|Dataset|Topic|string; | |||
// destination: Bucket|Dataset|Topic|string; | |||
// tslint:disable-next-line no-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.
@JustinBeckwith can you add a comment here indicating why any is being used, and what would be an ideal type definition if it were not for the npm/typescript limitations.
Fixes #429. This PR includes two changes:
any
in places where the types were previously usedI don't feel great about this fix. This another case where I'm starting to wonder if packaging types with the package is actually a good idea. Thoughts/ideas on the approach welcomed