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

[expo-av] Add return type to loadAsync() #7559

Closed
wants to merge 4 commits into from
Closed

[expo-av] Add return type to loadAsync() #7559

wants to merge 4 commits into from

Conversation

awinograd
Copy link
Contributor

@awinograd awinograd commented Mar 31, 2020

NOTE/EDIT

I just realized this was happening because in SDK 36, the PlaybackStatus type wasnt exported so i was importing directly from expo-av/src/AV. If I switch to import from the top level package expo-av then the compliation error goes away. That said, I believe this code change is still correct

Why

In my project I use typescript's noImplicitAny: true option. It causes loadAsync to show as a typechecking error.

node_modules/expo-av/src/AV.ts:227:3 - error TS7010: 'loadAsync', which lacks return-type annotation, implicitly has an 'any' return type.                    
                                                                                                                                                              
227   loadAsync(source: AVPlaybackSource, initialStatus: AVPlaybackStatusToSet, downloadAsync: boolean);                                                      
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 

How

The docs say that loadAsync returns a Promise. https://docs.expo.io/versions/v37.0.0/sdk/av/#playback-api

Test Plan

If I use this patch locally, the compilation error goes away

@bbarthec
Copy link
Contributor

bbarthec commented Apr 1, 2020

Hello @awinograd 👋 Thank you for your contribution! 🎉
To have your contribution fully mergable I need you to do the followings:

  • run yarn & yarn build in packages/expo-av to regenerate .d.ts files according to your changes
  • add entry in packages/expo-av/CHANGELOG.md according to the style that you can find in main CHANGELOG.md in repository 😉

@bbarthec
Copy link
Contributor

bbarthec commented Apr 3, 2020

@awinograd, sth went really bad on CI, cause it cannot access your repo 😞🤔
May I ask you to rebase on master and we'll see if the problem persists. If so, I would take over and fix it on my own 😉

@awinograd
Copy link
Contributor Author

@bbarthec I originally did a shallow clone of the repo so perhaps that was the issue? Anyway, I just rebased

@bbarthec
Copy link
Contributor

bbarthec commented Apr 6, 2020

@awinograd, everything seems almost perfect, but yarn lint failed on expo-av package now 😞
I need you to run yarn lint in expo-av and fix the error you'll get.
That should make required sdk CircleCI pipeline togo green 🎉

@bbarthec
Copy link
Contributor

bbarthec commented Apr 7, 2020

Replaced with #7704 as CI errors prevents from merging it from here 😞

@bbarthec bbarthec closed this Apr 7, 2020
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