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

feat(TS): auto-generate TypeScript definitions and add typecheck script #176

Merged
merged 3 commits into from
Nov 26, 2020

Conversation

kentcdodds
Copy link
Owner

What: auto-generate TypeScript definitions and add typecheck script

Why: To improve TypeScript support for packages using kcd-scripts. I think this is the last missing piece.

How: Update build (both babel and rollup) to add a tsc script to build the definitions.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #176 (22fe997) into master (a1b0371) will increase coverage by 0.47%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   84.59%   85.07%   +0.47%     
==========================================
  Files          17       17              
  Lines         422      422              
  Branches      164      162       -2     
==========================================
+ Hits          357      359       +2     
+ Misses         57       56       -1     
+ Partials        8        7       -1     
Impacted Files Coverage Δ
src/utils.js 86.02% <33.33%> (-1.76%) ⬇️
src/run-script.js 97.72% <100.00%> (ø)
src/scripts/pre-commit.js 100.00% <100.00%> (+18.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1b0371...22fe997. Read the comment docs.

@@ -0,0 +1,11 @@
{
Copy link
Owner Author

Choose a reason for hiding this comment

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

I welcome feedback on this shared config. I think most folks should be able to simply do:

{
  "extends": "./node_modules/kcd-scripts/shared-tsconfig.json",
  "include": ["src/*"]
}

But I may be mistaken.

Copy link

Choose a reason for hiding this comment

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

I'd suggest adding "forceConsistentCasingInFileNames": true which

Disallow inconsistently-cased references to the same file.

And assumeChangesOnlyAffectDirectDependencies: true which

When this option is enabled, TypeScript will avoid rechecking/rebuilding all truly possibly-affected files, and only recheck/rebuild files that have changed as well as files that directly import them.

This can be considered a ‘fast & loose’ implementation of the watching algorithm, which can drastically reduce incremental rebuild times at the expense of having to run the full build occasionally to get all compiler error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the include can even be left out in project-specific ts-config if we include it in shared-tsconfig, no? 🤔

People can then override include if they want to in their project-specific ts-config if they want.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks @eddyw,

at the expense of having to run the full build occasionally to get all compiler error messages.

Seems like it would be confusing if we enable this and someone runs typecheck once and gets some error messages, and then they run it again and they don't 🤔

I'm not sure I understand forceConsistentCasingInFileNames.

Thank you @MichaelDeBoey,

I was worried that include would be relative to the shared config, not the extending config. I'll do a little test to be sure.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yup, my suspicions were confirmed:

image

I suppose I could try ../../src/**/*... That actually should work fine... I think I'll do that.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to enable forceConsistentCasingInFileNames but not assumeChangesOnlyAffectDirectDependencies. We can alter that in the future if I learn to trust it.

Copy link

Choose a reason for hiding this comment

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

I'm not sure I understand forceConsistentCasingInFileNames.

It's like the most valuable option in tsconfig 😆 (joking)
It warns the developer if there is inconsistent casing in the filenames when importing or require

For instance, if you have thisExample.ts but you do import {} from './ThisExample.ts, it'll warn because the casing of the filenames is inconsistent. While this code will likely run in Windows, it'll surely break in Linux / MacOS.

I was worried that include would be relative to the shared config, not the extending config. I'll do a little test to be sure.

The default value for include is **/* if files option isn't specified. Maybe it's a sane default and you could leave it as is?

Also, I'd suggest removing exclude since the default value is actually better:

Default: ["node_modules", "bower_components", "jspm_packages"], plus the value of outDir if one is specified.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for that! I only want to include the src files because those are the ones that I want to generate type defs for. People can override if they need.

I'm pushing a fix to remove exclude. Thanks!

@kentcdodds kentcdodds merged commit 22137f3 into master Nov 26, 2020
@kentcdodds kentcdodds deleted the pr/add-typescript-defs branch November 26, 2020 14:55
@kentcdodds
Copy link
Owner Author

@all-contributors please add @eddyw for review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @eddyw! 🎉

@kentcdodds
Copy link
Owner Author

@all-contributors please add @rbusquet for review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @rbusquet! 🎉

@kentcdodds
Copy link
Owner Author

@all-contributors please add @MichaelDeBoey for review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @MichaelDeBoey! 🎉

@kentcdodds
Copy link
Owner Author

@all-contributors please add @Aprillion for review

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @Aprillion! 🎉

@github-actions
Copy link

🎉 This PR is included in version 7.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants