Skip to content

Conversation

danmontgomery
Copy link

@danmontgomery danmontgomery commented Oct 13, 2022

This fixes compilation errors introduced when types all moved to devDependencies - described in #883

By default, there will still be some errors because of default imports, which are all fixed by setting esModuleInterop to true (maybe worth documenting, or addressing in a separate PR?)

npm init -y
npm install typescript
npm install @kubernetes/client-node
echo 'import "@kubernetes/client-node"' > index.ts
npx tsc index.ts

Before:

node ➜ /workspaces/k8s-client $ npx tsc index.ts
node_modules/@kubernetes/client-node/dist/attach.d.ts:1:23 - error TS2688: Cannot find type definition file for 'node'.

1 /// <reference types="node" />
                        ~~~~

node_modules/@kubernetes/client-node/dist/attach.d.ts:2:23 - error TS2688: Cannot find type definition file for 'ws'.

2 /// <reference types="ws" />
                        ~~

node_modules/@kubernetes/client-node/dist/attach.d.ts:4:25 - error TS2307: Cannot find module 'stream' or its corresponding type declarations.

4 import stream = require('stream');

...

Found 166 errors in 71 files.

After:

node ➜ /workspaces/k8s-client $ npx tsc index.ts --esModuleInterop
node ➜ /workspaces/k8s-client $ 

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 13, 2022
@brendandburns
Copy link
Contributor

@danmontgomery thanks for the PR, but from my reading of this npm document:

https://docs.npmjs.com/specifying-dependencies-and-devdependencies-in-a-package-json-file

It seems that @typings files should not be included in the dependency section as they are not required for production, they are only required for development.

I'm open to arguments that I'm wrong, but this is the best source I could find on how package.json should be set up.

@danmontgomery
Copy link
Author

danmontgomery commented Oct 20, 2022

@brendandburns That's true, except when publishing a typescript library... "production" in this case are users of the package (anyone adding @kubernetes/client-node to their package.json). With these types not included in dependencies, they aren't grabbed by npm/yarn when installing the package, resulting in the errors posted above (and in the issue I linked to), so developers using this package need to manually add these undocumented dependencies to their own package.json

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#dependencies

The alternative (https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#publish-to-types) would be to submit a PR to https://github.com/DefinitelyTyped/DefinitelyTyped declaring type dependencies :)

@brendandburns
Copy link
Contributor

@danmontgomery thanks for the additional details. I'm ok to merge this.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, danmontgomery

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9adcd1d into kubernetes-client:master Oct 22, 2022
@rbnayax
Copy link
Contributor

rbnayax commented Oct 23, 2022

@brendandburns thanks for fixing both typescript issues (Im referring to the types dependencies). When can we expect a new version? We are eager to start working with the library and this is currently a show stopper for us :-)

@matteosilv
Copy link

Any news on the release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants