Migrate to TypeScript

[dgreif]

This adds TypeScript with js backward compatibility. As I am getting started on addTransceiver, the lack of types is pretty jarring. Eventually, we could have each of the iosrtc classes implement the corresponding interface from lib.dom.d.ts, which ensures this plugin is feature-complete with what users expect in the browser. For now, I converted Sender/Receiver/Transceiver as a first step. Also added @typescript-eslint and cleaned up some small lint issues it found.

Testing

To test calebboyd:typescript branch

cordova plugin remove cordova-plugin-iosrtc --verbose
cordova plugin add https://github.com/calebboyd/cordova-plugin-iosrtc#typescript --verbose
cordova platform remove ios --no-save
cordova platform add ios --no-save

[hthetiot]

Do you really need Typescript?

[hthetiot]

I don't see any value to the Typescript in the change you made so fare, the data argument you removed may be needed for the shim later.

[dgreif]

I think TypeScript adds a number of benefits here

• Type safety when working in the repo. This is useful for any dev diving in for the first time, or for experienced devs that want easy access to the native types for WebRTC. In the addTransceiver case, it will be really nice to be able to add class Sender implements RTCRtpSender and get IDE auto complete as fields are added, along with type checking to ensure all fields of the real RTCRtpSender are implemented properly.
• Access to modern javascript features, like object spread, default arguments, classes, etc. which may not be available in Safari 9. I set typescript to compile to es5, so it will compile a number of these newer constructs into code that works just fine in older browsers. I'm keeping a close eye on the suggestions in Looking for new maintainers #353 (comment), which includes Modernize the code to be more ES6 friendly, which is much easier to do with typescript in the mix

I agree with you assessment that Typescript didn't add a whole lot of value yet, but that was kind of intentional as I only converted a couple small files. Even in those files, I was able to transition to classes instead of old school function prototypes, and was able to use private properties on those classes.

Regarding the data arguments, having the argument there or not really doesn't make a difference if it isn't used. They can always be added back later if needed. Eslint pointed out these unused variables, which is the main reason I cleaned them up.

[hthetiot]

Change all to Typescript then but not half, still this is not required to SHIM Transceiver I think.

[hthetiot]

It will also require change to 6.1.x cause change more than minor if require Typescript.

[dgreif]

It's definitely not required, but will be very helpful in the process and sets the project up for full conversion down the road. I can convert everything, but that would have a major impact on any open PRs/branches as they would all need to be converted to TypeScript before merging. I've gone through JS -> TS conversions on other projects and it's much easier if you can do it one piece at a time.

Regarding 6.1.x, I'm not sure I understand your argument. Typescript is a dev dependency only, and has no impact on projects that use the plugin in production. The final code output by npm run build is pure JS and will run in Safari 9.

[dgreif]

@hthetiot any more thoughts on this? I'd really like to move forward with typescript as I implement the interface changes. I can start working to migrate the rest of the files if you think that is absolutely necessary, but like I said above, it will definitely cause merge conflicts for any open PRs.

[hthetiot]

I would prefer full Typescript over partial Typescript.

Depending the compilation to ES5 result we can release in 6.1.x due possible breaking changes (some JavaScript to Typescript may require debug/test mainly MediaStream)

[dgreif]

I started on the full TypeScript conversion yesterday and the MediaStream file is definitely tricky. I'll do my best to keep the current behavior. Hopefully I can push the rest today or tomorrow

[dgreif]

@hthetiot full TypeScript conversion is complete (except for util files, which I don't intend to convert for now). A good portion changes were cosmetic, like var -> const, or function () -> () =>. Definitely uncovered a couple bugs, but nothing major. I also added type definitions for all of the events coming from the swift side, which should significantly help anyone understand what type of data is coming back in those events. Let me know if you find any issues, but everything is working great in the iosrtc sample app.

Best way to view the changes will be without whitespace diffs: 804fc39?diff=split&w=1

[hthetiot]

First thank you.
Just to let you know I can confirm this change a lot of the implementation confirming that it will be a major release aka 6.1.0

[hthetiot]

Make sure this still works.

[hthetiot]

ok hopefull TypeScript extends and super() take are of that.

[dgreif]

It works on all of them except for MediaStream. Working on a fix.

[hthetiot]

ah, you may add "EventTarget.call(stream);" again then.

[dgreif]

Fixed in 0f29fc0. Not quite as simple as before because of the inheritance differences, but it works well.

[hthetiot]

	/**
	 * Additional, unimplemented members
	 */
	onaddtrack = null;
	onremovetrack = null;

Events like this are actually implemented via yaeti see https://github.com/ibc/yaeti/blob/master/lib/EventTarget.js#L101
meaning onaddtrack = function () {} will be trigger by dispatchEvent(new Event('addtrack'))

[hthetiot]

@dgreif see 1f28714

[hthetiot]

// THe Objective-C wrapper of WebRTC is old and does not implement .urls
source: https://github.com/calebboyd/cordova-plugin-iosrtc/blob/typescript/js/RTCPeerConnection.ts#L906

It does since a while now, so we may be able to remove fixPcConfig :
https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/src/PluginRTCPeerConnectionConfig.swift#L132

[hthetiot]

Regarding 6.1.x, I'm not sure I understand your argument. Typescript is a dev dependency only, and has no impact on projects that use the plugin in production. The final code output by npm run build is pure JS and will run in Safari 9.

Logic Implementation change too much for some parts, release will be 6.1.X, this will allow users that want to stay on stable previous architecture to have it stay like that. compare the file in www even if ES5 this is not the same ES5 that is generated.

Notice that for user/developer, they will not have acess to the TypeScript because its a SHIM and compiled, making it not usefull for consumer of the lib but only maintainers. using @types/webrtc for the developers that want to use TypeScript on there side.

[dgreif]

@hthetiot I'm fine with bumping to 6.1. I think I covered everything from above. When you say FULL typescript conversion, do you mean you want the js files in the extra directory converted as well? If so, I'd prefer to do those separately and get this merged asap to avoid more merge conflicts

[dgreif]

@hthetiot not sure if you saw my last message, but this should be ready for final review, as long as you are ok with the extra directory not being converted yet.

[hthetiot]

@hthetiot not sure if you saw my last message, but this should be ready for final review, as long as you are ok with the extra directory not being converted yet.

I'm fine keeping extras as pure JS, it's more easy to debug the *-tests.js in pure JS.

[dgreif]

Sounds good, I agree. In that cause, this pr should be ready for final review.

[hthetiot]

LGTM

but will need 6.0.16 release and will create V6.0 branch for maintenance LTS on 6.0.X like we have on V5 before merge on master

[hthetiot]

Full disclosure, the announcement that 14.3 will include native gUM in WKWebView significantly changes our approach
Even if WKWebView support gUM, the current safari implementation as limitation and issues that this plugin is handling, for example you cannot have more than 2 MediaSteam that play simultaneously last time I tested on ios safari.
Also we have no clue what WebRTC version apple will include, this plugin allow to have controls on the WebRTC version used. Not to mention that many Apple device cannot update to iOS 14.

So please yes finish typeScript at least, regarding the addTransceiver, you made a bit of a drama for it, so yes pls finish that if you can.

[hthetiot]

Let me know if you want me to perform conflicts resolve for you @dgreif

[dgreif]

@hthetiot I'm happy to take care of it on Monday. If you would like to get this merged before then, you are welcome to tackle it yourself. Just let me know either way.

[hthetiot]

I still have an issue with MediaStreamTrack end/inactive event on master that I'm may check this wk.

[dgreif]

@hthetiot I've rebased on top of your changes from earlier today. I had to rebase because merging after file rename doesn't work well. Also tested in the sample app and all looks good.

[hthetiot]

@hthetiot I've rebased on top of your changes from earlier today. I had to rebase because merging after file rename doesn't work well. Also tested in the sample app and all looks good.

Thanks you, I'm still chasing an issue with MediaStreamTrack end event for 6.0.17 as promised once resolved I will merge.

[hthetiot]

@dgreif 6.0.17 got release we going 6.1.0 now with this PR, can you merge master pls.
Otherwise i can try to do it, let me know.

[dgreif]

@hthetiot I'd love to get this merged, but it's a major pain to keep rebasing. This is why doing a smaller TypeScript change to start with and then doing more files over time is an easier approach. I can rebase it one more time, but it would be great to do it when you are at a relative stop on the rest of the code. Let me know when that is and I will get it updated.

[hthetiot]

@hthetiot I'd love to get this merged, but it's a major pain to keep rebasing. This is why doing a smaller TypeScript change to start with and then doing more files over time is an easier approach. I can rebase it one more time, but it would be great to do it when you are at a relative stop on the rest of the code. Let me know when that is and I will get it updated.

@dgreif i will take over no worries.

[hthetiot]

No need typescript for a Javascript SHIM, ES6 is enought and more easy to debug anyway.