aurelia / testing

Simplifies the testing of UI components by providing an elegant, fluent interface for arranging test setups along with a number of runtime debug/test helpers.
MIT License
40 stars 27 forks source link

fix(typings): improve typings #75

Closed mjwwit closed 6 years ago

mjwwit commented 7 years ago

Closes #72

CLAassistant commented 7 years ago

CLA assistant check
All committers have signed the CLA.

EisenbergEffect commented 7 years ago

@mjwwit Can you remove everything except the src folder from this PR? I'll generate the rest on release. Thanks!

mjwwit commented 7 years ago

@EisenbergEffect done.

EisenbergEffect commented 7 years ago

@mjwwit As a TypeScript user, would these changes constitute a breaking change? In other words, if you had tests written with this API before this PR was merged and then you updated to a version of the testing library after it was merged, would you have to fix your TS code. For example, would you have to go add generics? I just want to best understand the implications here and make sure we version appropriately.

mjwwit commented 7 years ago
  • Fix incorrect typings for waitFor, waitForElement, and waitForElements (#72)

This change makes a parameter optional, which is not a breaking change.

  • Add missing typing for ComponentTester.detached

Adding a missing type is not a breaking change.

  • Fix incorrect return types for bind, attached, and unbind (return value is a Promise)

The return types for these functions were changed from void to Promise<void>. Changing a return type can be a breaking change, but since the return type was void, it's not used. And if it is used, it would be used in a promise or async chain, wrapping the void in a Promise. Hence, changing the return type to Promise<void> will not cause any issues.

  • Make ComponentTester generic, so a contained view-model type can be passed in

This is currently a breaking change, since a generic type in TypeScript does require implementations to specify the type parameter(s). However, I could pass it a default (any) so it would work as it normally would before this fix if no type parameter is specified.

  • Fix return type of dispose

The return type of dispose was any. This means that it could technically be used as any type, even an incorrect one. If this is the case, it may result in a compile-time error. Since you'd have to manually specify an incorrect type to cast to, the error would be desired behavior in my opinion. This may be a breaking for the very few people that actually use the return value of dispose, and choose to use it with an incorrect type.

  • Change bindingContext type from any to {}, as it has to be an Object

This change would only cause an error if something other than an object is passed as a bindingContext. This is not possible to begin with as aurelia.enhance (aurelia-framework) requires the bindingContext to be an Object. I've chosen to use {} instead of Object, for reasons explained here.

mjwwit commented 7 years ago

I just noticed there's something weird going on with the resulting .d.ts file. It somehow loses some of the type information (e.g. default generics, and some optionals). I'll try to figure out why this is happening.

mjwwit commented 7 years ago

So it appears TypeScript is not used to generate the .d.ts file. A babel plugin is used instead. A flag inside the build/paths.js file changes this behavior.

However, when using TypeScript to generate the .d.ts file, some other issues come to light;

  1. The project's TypeScript version is very old (1.9), and does not support default values for generics. This is supported from TypeScript versions 2.3 and up.
  2. After upgrading the TypeScript dependency version and using it to generate the .d.ts file, the project seems to be riddled with type errors.

I can switch the build flag, upgrade TypeScript, and fix all of the type errors without any major impact on the generated JavaScript code. This PR will be quite a bit bigger if I do, however. How would you like me to proceed?

EisenbergEffect commented 7 years ago

Let's hold on this for now. This might be a good opportunity to just port this library to TypeScript. It's very large, so the effort wouldn't be too big. @jdanyow What's the state of our standard TS build setup? What's the best way to set that up? Do we just copy what is done in validation? Any missing pieces there?

mjwwit commented 7 years ago

@EisenbergEffect That was actually something I was going to suggest, but it seemed a bit too big for a first contribution. The whole TypeScript syntax inside .js files doesn't play well with most IDEs and combining the TypeScript and Babel transpilers seems overly complex.

EisenbergEffect commented 7 years ago

When we made this choice...almost three years ago, things were quite a bit different. I think TS is the way to go today, so I'm trying to gradually get things moved over. This might the right time to do it for this library. Just need to confirm with @jdanyow on the repo setup. If that is good, I can create a branch for the conversion. @mjwwit Do you have any interest in contributing that? It is a larger task. Let me know what you think.

mjwwit commented 7 years ago

@EisenbergEffect Absolutely. I'm actually already looking at aurelia-validation.

mjwwit commented 7 years ago

@EisenbergEffect I managed to get everything working with the TS build from aurelia-validation with only a few minor changes. The only thing I'm unsure of are the docs. Even though I think they're mostly correct, there may be a few things missing or wrong.

Let me know when you've created a branch so I can send a PR.

EisenbergEffect commented 6 years ago

Sorry this took so long...lots of stuff going on for me now :) I've created a branch called typescript-port. If you would like to go to work on that, that would be awesome! I'm going to close this PR for now in favor of the TS rework in that branch. Looking forward to seeing it!