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: fix typescript compilation #295

Merged
merged 2 commits into from
Dec 7, 2018
Merged

fix: fix typescript compilation #295

merged 2 commits into from
Dec 7, 2018

Conversation

JustinBeckwith
Copy link
Contributor

Fixes #287. There are a few places where we accept File type objects, which is defined in google-cloud/storage. The package is only needed for types, so it's installed as a devDependency. However - this means the types for storage aren't shipped with the module, which causes compilation to fail for consumers of the package. This change dumps the File type for now in favor of any, and adds an install test to verify basic compilation works after an npm pack.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 7, 2018
@JustinBeckwith
Copy link
Contributor Author

@googleapis/node-team this is an interesting problem, and I'm interested in y'alls opinions :)

@jkwlui
Copy link
Member

jkwlui commented Dec 7, 2018

Let’s define an interface that contains the properties of File that this library needs. Since we only intend to access the relevant properties and we’re not creating a new File object, we should just provide the interface and note in the docs that a File object can be provided.

@ofrobots
Copy link

ofrobots commented Dec 7, 2018

I see this problem with TypeScript code once per fortnight. The real solution here would be a way to tell the package installer this info: "If you install this package as a devDependency of a top-level install, you also need to install these other devDependencies of mine".

IOW, we need a transitiveDevDependencies section in package.json that somehow npm could respect. It might be worth engaging with TypeScript and npm folks on this.

@JustinBeckwith JustinBeckwith merged commit d80fe54 into googleapis:master Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants