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

Improve TypeScript #66

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Improve TypeScript #66

merged 4 commits into from
Jul 24, 2023

Conversation

matthew-e-brown
Copy link
Contributor

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
Copy link
Contributor Author

matthew-e-brown commented Jul 23, 2023

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
Copy link

codecov-commenter commented Jul 23, 2023

Codecov Report

Patch coverage: 98.03% and project coverage change: -1.89 ⚠️

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

❗ 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
@@             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 Coverage Δ
src/ssh-config.ts 97.96% <97.96%> (ø)
index.ts 100.00% <100.00%> (ø)
src/glob.ts 100.00% <100.00%> (ø)

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

src/ssh-config.ts Outdated Show resolved Hide resolved
tsconfig.json  -  set declaration to true
package.json   -  replace "src" with "src/*.js" in files array
package.json   -  add .d.ts files to files array
.eslintrc      -  ignore generated .d.ts files
.gitignore     -  ignore generated .d.ts files
- Make capitalization and periods consistent in function doc comments
- Update `compute` params to `host: string`

Thankfully the function and parameter names are self-documenting enough
that the doc comments needn't be more than a sentence.
@matthew-e-brown
Copy link
Contributor Author

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.

This makes them match `.compute`.
@matthew-e-brown matthew-e-brown requested a review from cyjake July 23, 2023 17:18
@cyjake
Copy link
Owner

cyjake commented Jul 24, 2023

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

@cyjake cyjake merged commit 0c0f37d into cyjake:master Jul 24, 2023
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.

3 participants