search

angular / ts-api-guardian

MIT License
15 stars 15 forks source link

feat: add support for new `TypeScript` versions #25

Closed alan-agius4 closed 6 years ago

alan-agius4 commented 6 years ago

There are several changes:

Closes: #23

alan-agius4 commented 6 years ago

@alexeagle can you have a look please?

alan-agius4 commented 6 years ago

It would be great if you can test it with angular as at the moment I don’t have my mac handly until the weekend and on my windows I always have problems to run gulp update:api inside of angular

Thanks

alexeagle commented 6 years ago

I tried this out in the angular repo and it produced a ton of deltas. I tried to turn them into a PR against angular/angular but something is broken with the gulp public-api:update script in angular, it just hangs (not sure if that is related to this change) https://pastebin.com/xBEPaaz5 is a sample of the delta.

Is it expected that this PR would change the public API golden files so much?

alan-agius4 commented 6 years ago

Thanks for taking the time into looking into this.

No, those changes are not expected. To be honest they are mainly ordering related and are based on the following; https://github.com/angular/ts-api-guardian/blob/master/lib/serializer.ts#L268

I'll have a look at them and see what's the issue.

alan-agius4 commented 6 years ago

Hey, I pushed an update that should fix the ordering. I also added a Unit Test to cover this. https://github.com/angular/ts-api-guardian/pull/25/commits/e088033ef5d794d6d6a55372400f24f1c88699b9 Can you try to have a another look?

Thanks a lot.

alan-agius4 commented 6 years ago

Hey @alexeagle, I’ve tested it with Angular repo, and it’s looking good. i created a PR and tagged you.

alexeagle commented 6 years ago

Awesome, thank you for making this mergable. FYI after Angular v6 is released we plan to move this repo into angular/angular, which will make this easier since the Angular API update will be part of the CI for changing ts-api-guardian

alan-agius4 commented 6 years ago

Thanks for the info 😁, it does make sense indeed to move it to angular/angular and maybe it will get maintaned a bit more. Cause I don’t think many people know about this.