Publish TypeScript types, user newer class syntax#104
Merged
ErikBoesen merged 8 commits intomainfrom Oct 13, 2022
Merged
Conversation
This converts a number of comments in the code to JSDoc-style comments (and adds some more detailed descriptions/types along the way) and uses TypeScript to build type definition files from the comments. This allows you to publish types directly from this package instead of making TypeScript user install `@types/datadog-metrics`, which is maintained separately, by hand, and won't always be up-to-date. It does not add new comments for objects that are not publicly exposed (although that would be a good idea for consistency). It also adds NPM lifecycle scripts to automatically build types when publishing. Fixes #86.
This should help verify that most changes are valid.
This updates the syntax for all the public classes (reporters, BufferedMertricsLogger) to use native `class` syntax. This allows TypeScript to build better type definitions (compare the output before and after this commit). It also makes for slightly more compact and easy-to-manage syntax. Partially fixes #83.
This extends the previous change to use class syntax and JSDoc comments everywhere in the codebase. It also turns on type checking (which now works because we are using class syntax everywhere).
The prepare script runs when installing, which causes errors when running tests, and means we see type errors multiple times over in CI. This moves it to prepack so it only runs when packing and/or publishing.
Collaborator
Author
|
We also get nice type checks in CI now! This is what shows up in the "files" tab of a PR (I made this error intentionally to test it):
|
This probably shouldn't have gotten in originally, but it's pretty noisy when all tests run twice when pushing new commits to a PR (once for the push event and once for the update to the PR).
Mr0grog
commented
Oct 13, 2022
Collaborator
Author
Mr0grog
left a comment
Mr0grog
left a comment
There was a problem hiding this comment.
@ErikBoesen This should be good to go! Please review if you have any thoughts. It’s a big change, but is mostly just formatting and comments.
Comment on lines
+3
to
+5
| push: | ||
| branches: | ||
| - main |
Collaborator
Author
There was a problem hiding this comment.
This isn’t technically relevant to this PR, but I probably should have set it up this way to begin with. (Every test action is currently running twice on each push to a PR right now — once for the push and once for the update to the PR — and this makes them only run once).
Comment on lines
+23
to
+39
| /** | ||
| * Wrap a function so that it gets called as a method of `sharedLogger`. If | ||
| * `sharedLogger` does not exist when the function is called, it will be | ||
| * created with default values. | ||
| * @template {Function} T | ||
| * @param {T} func The function to wrap. | ||
| * @returns {T} | ||
| */ | ||
| function callOnSharedLogger(func) { | ||
| // @ts-expect-error Can't find a good way to prove to the TypeScript | ||
| // compiler that this satisfies the types. :( | ||
| return (...args) => { | ||
| if (sharedLogger === null) { | ||
| init(); | ||
| } | ||
| return func.apply(sharedLogger, args); | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
This isn’t strictly necessary, but it means we don’t have to specify the types for all the top level functions (flush, gauge, increment, etc.) twice. Because the previous setup referenced functions as strings, TypeScript couldn’t figure out the signatures from the BufferedMetricsLogger class and copy them. With this setup, it can.
For example, the old style generated types like:
export declare const flush: any;
export declare const gauge: any;
export declare const increment: any;
export declare const histogram: any;
export declare const distribution: any;
export declare const BufferedMetricsLogger: typeof loggers.BufferedMetricsLogger;
export { reporters };But this gets us real signatures:
export declare const flush: (onSuccess?: () => void, onError?: (error: Error) => void) => void;
export declare const gauge: (key: string, value: number, tags?: string[], timestampInMillis?: number) => void;
export declare const increment: (key: string, value?: number, tags?: string[], timestampInMillis?: number) => void;
export declare const histogram: (key: string, value: number, tags?: string[], timestampInMillis?: number, options?: any) => void;
export declare const distribution: (key: string, value: number, tags?: string[], timestampInMillis?: number) => void;
export declare const BufferedMetricsLogger: typeof loggers.BufferedMetricsLogger;
export { reporters };
Comment on lines
+43
to
+50
| }, | ||
| "files": [ | ||
| "README.md", | ||
| "LICENSE", | ||
| "index.js", | ||
| "lib/**", | ||
| "dist/**" | ||
| ] |
Collaborator
Author
There was a problem hiding this comment.
This includes fewer files than we used to, but that’s probably a good thing. The tests and lint configurations and so on all used to get published, and no longer do with this configuration.
ErikBoesen
approved these changes
Oct 13, 2022
ErikBoesen
deleted the
86-we-can-publish-type-definitions-without-making-this-typescript
branch
Mr0grog
added a commit
that referenced
this pull request
Oct 13, 2022
I missed adding release notes in #104, so I'm adding them separately here. This also cleans up some extra blank lines I accidentally committed.
Mr0grog
added a commit
that referenced
this pull request
Oct 13, 2022
Mr0grog
added a commit
that referenced
this pull request
Feb 21, 2023
This is a follow-on to #104. I missed that this file would get published, even though it should not be.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This converts most of the function/class comments in the code to JSDoc-style comments (adding some more detailed descriptions/types along the way) and uses TypeScript to build type definition files from the comments. This allows us to publish types directly from this package instead of making TypeScript users install
@types/datadog-metrics, which is maintained separately, by hand, and isn’t always be up-to-date.It also converts old-style classes to the newer, ES6+ class syntax. The TypeScript compiler generates clearer and less redundant type definitions when classes are used, so this actually has a meaningful impact beyond just code style.
When building types, the TypeScript compiler can perform limited validation of the actual JavaScipt code (making sure it plausibly matches the types in the comments, and where types are specified, making sure the code matches what the types say can be done). I’ve turned that on and added a CI step to generate types, so we should see errors if the doc comments change in an incorrect way, or if a function signature changes without also updating the doc comments describing the types.
Finally, I’ve added an NPM lifecycle script to automatically build types when publishing.
You can manually build types by running
npm run build-typesor clear out any generated code types by runningnpm run clean. The type definitions are build in thedistdirectory.Note this does not do type checking on the test files — that would be a good idea, but would require a bunch more changes, and this PR is already too big (e.g. the
shouldassertions don’t work, you need to useexpectinstead).Fixes #83, fixes #86.