Conversions to typescript and misc stuff #99
Conversation
We shouldn't have to support this on the receiver anymore I believe |
|
Lol it's failing the tests because there are no js files left :D |
2aabb83 to
a44475d
Compare
|
have made |
99a420a to
acefa6b
Compare
f371a14 to
ff27287
Compare
ff27287 to
bc63284
Compare
|
Tested this watching a movie and it worked allright :) |
src/components/fetchhelper.ts
Outdated
| @@ -1,12 +1,12 @@ | |||
| export function getFetchPromise(request) { | |||
| var headers = request.headers || {}; | |||
| export function getFetchPromise(request: any): Promise<any> { | |||
There was a problem hiding this comment.
| export function getFetchPromise(request: any): Promise<any> { | |
| export function getFetchPromise(request: never): Promise<never> { |
If it's 100% impossible to determine exactly what kind of objects we will get here, use never or unknown. As more things we have without strictly typing, more bugs can slip.
There was a problem hiding this comment.
These return a fetch response. I see tutorials using templating for the response type, for the arguments I suspect there is something ready to use.
| @@ -55,71 +62,77 @@ export class playbackManager { | |||
| ); | |||
| } | |||
|
|
|||
| async playFromOptions(options) { | |||
| async playFromOptions(options: any): Promise<boolean> { | |||
There was a problem hiding this comment.
This deserves a new interface as well
| } | ||
|
|
||
| playFromOptionsInternal(options) { | ||
| playFromOptionsInternal(options: any): boolean { |
| } | ||
|
|
||
| playNextItem(options, stopPlayer) { | ||
| var nextItemInfo = getNextPlaybackItemInfo(); | ||
| playNextItem(options: any = {}, stopPlayer = false): boolean { |
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| playPreviousItem(options) { | ||
| playPreviousItem(options: any = {}): boolean { |
| async playItem(item, options, stopPlayer) { | ||
| async playItem( | ||
| item: BaseItemDto, | ||
| options: any, |
1e61ea2 to
3ee3591
Compare
3ee3591 to
7e9a7db
Compare
|
I have decided that I will not be refactoring more I'm not against a followup PR to fix more of these linter issues, in fact this is what I would prefer to do. |
There was a problem hiding this comment.
As commented many times in chat, we can fix linting and add more typings in the future, otherwise this PR will be insane (it's already insane, but we don't want to make it more insane) :).
I checked your newly added typings and I would say the status of this PR is much better than what it was when I requested changes, so as long as it works and doesn't break the client completely, I would say it's good to go.
Ping me once all the client testing is done.
|
Okay, I believe at this point @ferferga is satisfied with the changes after his review, despite not dealing with everything. I will now stop any more updates to this PR to allow for testing :) |
|
Regression: Admin Dashboard->Active Devices does not report playback and does not load the minibar/remote control at the bottom |
Co-authored-by: Fernando Fernández <ferferga.fer@gmail.com>
7d96301 to
6e26f44
Compare
|
Okay.. I think I got that bug fixes, it was in helpers.ts: getSenderReportingData. After dropping the extend() call it was not a shallow copy anymore. The null & delete operations deleted that information for everyone I think. I figured it would be easier to not take a copy and live with sending some extra fields. |
|
I've tested this on a whole bunch of devices and am not finding any regressions. LGTM |
|
Asking for anyone that would like to look over the code once more or test it :) At this point it would be really nice to get it merged |
yarn servea thingPlease test throughly! A lot changed and regressions are possible..