cyjake / ssh-config

πŸ“Ÿ SSH config parser and stringifier
MIT License
78 stars 19 forks source link

Improve TypeScript #66

Closed matthew-e-brown closed 1 year ago

matthew-e-brown commented 1 year ago

I noticed when I first decided to take a look at this library for #65 that NPM does not show it as having declared types, nor does it have any in DefinitelyTyped. I was quite surprised to find out that this was despite the fact that it is written in TypeScript.

While I've never published a package with types, the TypeScript documentation on publishing makes it look like all you have to do is include types as a key in your package.json and point it to a .d.ts file. This PR updates tsconfig.json to emit declaration files, as well as updating the other rc/ignore/config files to exclude/include them as required. With this, the package should get its very own little TS badge! πŸ˜„

TS ← (this fella)

matthew-e-brown commented 1 year ago

Hmm... I just realized that I somehow managed to forget to actually add the types key in package.json amongst my other edits... Don't bother asking me how I did that πŸ€¦πŸ»β€β™‚οΈ.

That isn't a huge deal, since,

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

Also note that if your main declaration file is named index.d.ts and lives at the root of the package (next to index.js) you do not need to mark the types property, though it is advisable to do so.

But, like they say, it may be wise to do that... One second.

Edit: it looks like adding types: index.d.ts makes TSC interpret that file as being one of our handwritten TS files, and so it refuses to overwrite it ("refusing to overwrite input file" error). So... nevermind, we'll keep types out of package.json.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 98.03% and project coverage change: -1.89 :warning:

Comparison is base (ebbeb2b) 100.00% compared to head (53dd5b4) 98.11%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #66 +/- ## =========================================== - Coverage 100.00% 98.11% -1.89% =========================================== Files 2 3 +1 Lines 231 265 +34 Branches 0 74 +74 =========================================== + Hits 231 260 +29 - Partials 0 5 +5 ``` | [Impacted Files](https://app.codecov.io/gh/cyjake/ssh-config/pull/66?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chen+Yangjian) | Coverage Ξ” | | |---|---|---| | [src/ssh-config.ts](https://app.codecov.io/gh/cyjake/ssh-config/pull/66?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chen+Yangjian#diff-c3JjL3NzaC1jb25maWcudHM=) | `97.96% <97.96%> (ΓΈ)` | | | [index.ts](https://app.codecov.io/gh/cyjake/ssh-config/pull/66?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chen+Yangjian#diff-aW5kZXgudHM=) | `100.00% <100.00%> (ΓΈ)` | | | [src/glob.ts](https://app.codecov.io/gh/cyjake/ssh-config/pull/66?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Chen+Yangjian#diff-c3JjL2dsb2IudHM=) | `100.00% <100.00%> (ΓΈ)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

matthew-e-brown commented 1 year ago

Actually... looking at the match function, it looks like, while Match exec "command" is supported with a spawnSync, no tokens (%h, %L, %r, etc) are expanded. Do you have any desire to support that? Not just in Match exec but in things like IdentityFile, RemoteCommand, etc. (the TOKENS section of the man page lists the keywords that take arguments)?

It seems like it would be a bit of work to properly expand all those arbitrary tokens like "the user's home directory" and "the local hostname", it'd need a bunch of Node os calls or something to query a bunch of little details. So I would totally get it if that's a bit out of scope.

cyjake commented 1 year ago

nice catch about TOKENS, yep I think token interpolation can be left out of this pr.