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

Typescript declarations #188

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

spetrac
Copy link

@spetrac spetrac commented Aug 15, 2024

Because of issue #139 I created a simple solution to generate typescript declarations for the package.

These declarations will help developers to use the module more effectively, because they will get better code intellisence and typescript support if needed.

Declarations will only be generated automatically on packing/publishing and are excluded from git for now. On generation, the previous types folder will be cleared automatically and typescript will generate the new declarations. They can be generated manually of course.

There are minor changes to the package.json as well

  • to include the types in the generated package for npm
  • and to let typescript pick up the types as addition to the source code

Additionally to the declarations, typescript will also generate type-maps to make it easier for IDEs to match the related source code on code navigation.

Copy link
Contributor

@aljones15 aljones15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure on what the stance is with typescript support, but at the very least we need a solution that doesn't add 3 extra dependencies that the vast majority of users will never use.

@@ -64,6 +68,8 @@
"klona": "^2.0.6",
"mocha": "^10.7.0",
"mocha-lcov-reporter": "^1.3.0",
"rimraf": "^6.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is rimraf really a main dependency? https://www.npmjs.com/package/rimraf

This seems like an entry in devDepencies also do @types/node and typescript have to be in the dependencies for all users? It seems like that introduces a lot of bloat in this library. Are you sure those aren't devDependencies also?

Copy link
Author

@spetrac spetrac Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those definitively belong in the devDependencies and they are already.

You are also right that rimraf is not a necessary dependency and only used during development to clear up between builds. You could also use the usual rm command on linux, but then it would not be cross-plattform compatible. You can also discard the clear-up completely, because the build step is meant to be used automatically when publishing a package, but then it could be irritating during development.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The rimraf dependency is only for the convenience of the developer, so that previous builds must not be cleared manually during development. You can see it's usage in the package.json-build-types-script.
  • The typescript dependency is obviously necessary to build the types. You can see usage of tsc in the build-types-script.
  • The @types/node dependency is used to give typescript the necessary types in the NodeJS environment, so that it will not warn of missing types and validate the usage of internal classes and API.

These are just the basics of typescript development and should not really be a surprise. If you install the package in production those dependencies will not even be downloaded. The published types should then already be included in the package. Alternatively you can publish proper types in the DefinitelyTyped repository, but then you should not just use automatically generated types.

@spetrac
Copy link
Author

spetrac commented Oct 14, 2024

If you do not want extra dependencies, then just include manual types. From what I can expect from the maintainers I did not believe that they were willing to manually type their code or switch to typescript all along. Even then, @types/node would still be advised during development of declaration files.

If you need further information about typescript declarations, then read the documentation:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants