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

Add a way to validate the memory usage of your dts files #34

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

Conversation

orta
Copy link
Contributor

@orta orta commented Sep 13, 2019

Problem: We can verify types in definitely typed aren't causing slowdowns for folks with tooling on that repo. There are no tools for doing that anywhere else.

I'd like tsd to be the tool we recommend in the TS guides for how to create dts files for JS projects, as I think it's a simpler and more useful abstraction that dtslint for most projects. I'd like for people to be able to do two things with tsd: validate their types, and for larger projects - ensure they don't accidentally merge PRs which affect performance.

Not many libs should need a tool like this, but ones which offer presentation layers (e.g. CSS in JS libs) tend to have accidental drastic memory increases.


This PR adds 3 flags:

--verify-size - This stores a JSON blob with your index.d.ts which is the number
                         of types, and memory usage of typescript in running your project.
                         This is then used to make a comparison on the next run that the
                         sizes haven't drastically increased
--write - Always write the JSON blob
--size-delta - How much change is acceptable for you

You can see it in action against a fixture here:

Screen Shot 2019-09-13 at 1 22 01 PM


This PR doesn't have docs and tests yet, and access TS private APIs ( microsoft/TypeScript#33345 ) - but I'm opening it to start chatting about the structure and what you want tsd to be vs what I've modified it to be.

@Andarist
Copy link

Why this could not be incorporated into dtslint? Setting up library build, tests and everything is already somewhat difficult (needs certain skills, config knowledge etc) - adding yet another tool which can be used to verify smth seems like overhead in this case.

@orta
Copy link
Contributor Author

orta commented Sep 13, 2019

dtslint is built for definitely typed sized types of projects, but you can re-use it with some work. It's far too much infra for checking 2-3 d.ts files in a JS repo though. tsd is considerably simpler, and easier to use.

@weswigham
Copy link

dtslint does work on individual folders. dtslint-runner is our wrapper around is to handle things at the scale of DT. Imo, if dtslint is difficult to use, it aughta be improved. Then again, both tools are a bit flawed rn - they both use tslint which is EoL.

@Andarist
Copy link

if dtslint is difficult to use, it aughta be improved.

Exactly my thinking. You can even have two modes - a simple one and more robust one (if needed). Still would be easier to teach than 2 different tools.

@orta
Copy link
Contributor Author

orta commented Sep 13, 2019

tsd doesn't use TSLint under the hood, it uses the TS API. It really is a simpler, is less to learn and maintain for JS folks who don't need all of dtslint trivia. It replaces dtslint for people using JS - you don't need to learn two tools. #10

dtslint can have this feature too, nothing here stops that.

@weswigham
Copy link

weswigham commented Sep 13, 2019

tsd doesn't use TSLint under the hood, it uses the TS API.

Isn't that even worse? Our APIs are.... not great. They're very poweruser-focused.

@orta Wouldn't a more optimal thing be integrating this into a tool js users already use, like eslint? Since we have have semantic eslint rules now, isn't it possible, in theory, to just report these as part of an eslint rule?

@weswigham
Copy link

weswigham commented Sep 13, 2019

For example, the jest community has this eslint rule for managing snapshot sizes - I feel like this is in a similar vein (a lint rule about size/perf concerns).

@orta
Copy link
Contributor Author

orta commented Sep 13, 2019

Using tsc under the hood is an underlaying abstraction is fine, tsd uses a program to grab any diagnostics then checks if any live inside expectType expressions which are then used to type-check. Anything that fails outside of that is a real fail, and anything inside the expect's are thrown away and validated against the generics.

Eslint could work yep, though we'd have to build infra for validating dts files inside eslint maybe - which would mean either replicating dtslint's comment support or this tsd's technique inside eslint

@Andarist
Copy link

It replaces dtslint for people using JS - you don't need to learn two tools.

It's not like those are two exclusive worlds - i.e. I often help different OSS projects and sometimes they are written in TS, sometimes in JS. Latter sometimes keep their types in DefinitelyTyped, sometimes in the repository. Some might want to keep using dtslint, some might consider switching.

@orta
Copy link
Contributor Author

orta commented Sep 13, 2019

I'm not arguing for the deprecation of dtslint - people are welcome to use that but tsd has a smaller footprint on the repo (no need for tsconfig.json, tslint.json etc), uses normal-ish syntax (generics is probably intimidating though), is a small dependency where you see results of fails etc in your editor without config.

It's simpler, with less features but there's enough to make JS folk feel comfy that the dts file is tested and that they could see that themselves by opening the file in vscode - if people shipping dts files to other projects want to do things like test many versions of ts, or have it conform to the extra standards we have in dtslint then dtslint is still there. We make this recommendation in dtslint

Which is why I'd like to give those folks the ability to make sure that they can also be sure they're not accidentally making editors slow. dtslint can have the same feature.

@weswigham
Copy link

It's simpler, with less features but there's enough to make JS folk feel comfy that the dts file is tested and that they could see that themselves by opening the file in vscode

dtslint also doesn't have to have a tsconfig, your tests are probably just going to be bad unless you enable strictness settings and other bits. tsd is the same way.

@sindresorhus
Copy link
Collaborator

you enable strictness settings and other bits. tsd is the same way.

tsd is strict by default: https://github.com/SamVerschueren/tsd/blob/47c7796a65d8ecdeb31598b4cbfd7a39ab0777fe/source/lib/config.ts#L35

@weswigham
Copy link

Hah, totally missed that.

@SamVerschueren
Copy link
Collaborator

Hi @orta! I like this feature. Regardless that it can be ported to dtslint or any other tool, think it's a nice addition to tsd.

@sindresorhus @BendingBender feel free to give your thoughts on how this feature might/should work within tsd.

Options
--verify-size, -s Keep on top of the memory usage with a data snapshot
--write, -w Overwrite existing memory usage JSON fixture
--size-delta, -d What percent of change is allow in memory usage, default is 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kinda seems like these should be in a subcommand, like tsd memory-usage --write. For example, tsd --write is way too generic naming for the top-level interface. However, we can't really do subcommands as we expect paths. Alternatively, we could namespace the flags: --mem-write, --mem-verify-size, ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sindresorhus We could however do tsd --cwd=path instead of tsd path so we can still accept subcommands?

Or tsd verify path or something? Not sure, just thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to ideally make it run at the same time as normal tests (rather than have to run it as a 2nd command) - which is why I opted for using flags on the main command.

I copied --write from jest, but I'm open to having it more verbose and specific too 👍

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.

5 participants