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

Make typechecks work on tests too #109

Merged

Conversation

wolframkriesing
Copy link
Contributor

@wolframkriesing wolframkriesing commented Jan 16, 2020

I was playing around with the typing and realized the tests did NOT get type checked, so I fixed the tsconfig.json to do so and a good number of type errors showed up. I fixed them and learned a lot about how to use tsc efficiently with JS code, thanks for the learning opportunity! :)

  • make typechecks also run against the tests
  • load all @types modules needed for the libraries used (mocha, sinon, chai)
  • built a test-helpers.js which allows tsc to find the types and the browser to use them from window I am sure there are better ways, than the old style way I did it
  • fixed all the type errors (that was fun)

There are a couple // @ts-ignore I introduced where I didn't know how to fix the typing errors, maybe someone has some ideas.

@JanMiksovsky
Copy link
Member

Holy cow, this is amazing!

Getting typechecking working on tests has been our to-do list for a long time. I think the last time I tried to tackle that, I ran into trouble trying to find type declarations for our testing libraries, so I gave up and never got back to it.

I'll review as soon as I can, but for now just wanted to say thanks for doing this.

@wolframkriesing
Copy link
Contributor Author

I wanted to use elix to learn about typing pure JS files, basically the use of tsc as a type linter and realized while experimenting that it never throws for the tests ... and gladly I had that opportunity to learn using it by fixing all the types in the tests without learning how to set up all the basics. Glad it looks useful. I would be happy to learn if I was wrong in places or where types might not match.

@JanMiksovsky JanMiksovsky merged commit 865e703 into elix:master Jan 30, 2020
@JanMiksovsky
Copy link
Member

Once again, many thanks for submitting this PR so that we can have more complete test coverage.

Inspired by this, I've followed up with some additional work to remove some of the @ts-ignore comments you were forced to add. In some cases, that required rewriting the test a bit. In all cases, the rewritten tests are better, so this was a good exercise.

While doing this work, I realized that some of our manually-written .d.ts files declared properties that didn't exactly match the corresponding property definition in the .js file. Specifically, a number of .js files declared get properties (with no set), but the corresponding .d.ts declaration made it appear that the property was read/write. I've fixed that by adding readonly to those properties in the .d.ts files. E.g., ReactiveMixin.d.ts now defines the state as readonly [internal.state]: State, so that we can statically detect any attempt to overwrite that property. Once again, going back into the tests for this PR was a useful exercise for me too.

Anyway, thanks again.

@wolframkriesing wolframkriesing deleted the make-typechecks-work-on-tests-too branch January 30, 2020 23:05
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.

None yet

2 participants