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

Change react-native-video to a peer dependency with any version required #1070

Closed
wants to merge 6 commits into from

Conversation

ahartzog
Copy link

@ahartzog ahartzog commented Jan 7, 2019

Including react-native-video as a dependancy at ^3.2.1 caused a conflict with our existing usage of react-native-video. Error was: "Tried to register two views with the same name RCTVideo".

Changing react-native-video to a peer dependency resolves this issue.

I've also confirmed that the video feature works fine with the newest version of react-native-video.

We could alternatively set it as a peer dependency with version 3.x as well?

Copy link
Collaborator

@xcarpentier xcarpentier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you figured out why tests not working anymore?

@codecov
Copy link

codecov bot commented Jan 7, 2019

Codecov Report

Merging #1070 into master will increase coverage by 42.6%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1070      +/-   ##
==========================================
+ Coverage    43.1%   85.71%   +42.6%     
==========================================
  Files          21        3      -18     
  Lines         515       14     -501     
  Branches      112        3     -109     
==========================================
- Hits          222       12     -210     
+ Misses        221        2     -219     
+ Partials       72        0      -72
Impacted Files Coverage Δ
src/utils.js 75% <0%> (-12.5%) ⬇️
src/MessageImage.js
src/GiftedAvatar.js
src/Time.js
src/Message.js
src/Day.js
src/InputToolbar.js
tests/context.js
src/Composer.js
src/GiftedChat.js
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd69b2...a943652. Read the comment docs.

@ahartzog
Copy link
Author

ahartzog commented Jan 7, 2019

I'm actually working on some additional improvements to this right now, so I'm going to let it sit for now.

For example, there's currently no way to control the video on android! So I'm either going to pull in react-native-video-controls or allow a custom video player prop to be passed into the video message object.

@ahartzog ahartzog closed this Jan 8, 2019
@ahartzog
Copy link
Author

ahartzog commented Jan 8, 2019

I'm going to re-open or create a new one when I have a complete solution.

@xstable
Copy link
Contributor

xstable commented Jan 25, 2019

I'm actually working on some additional improvements to this right now, so I'm going to let it sit for now.

For example, there's currently no way to control the video on android! So I'm either going to pull in react-native-video-controls or allow a custom video player prop to be passed into the video message object.

I've worked at the same task today (controls in Android) and kicked "react-native-video-controls" because it is outdated - even like react-native-video-player

Finally I've figured out, that you now can handle video-controls by react-native-video.
Currently you still have to patch, but I think in notime, the merge will be done.
I've used patch-package to try it, and it works really well on Android (expected the know Bug with the Play-button... thats why it's not yet merged).
If @IbrahimSulai have fixed it, I'm sure it will be merged quickly.

TheWidlarzGroup/react-native-video#1414

Would really appreciate if you will continuou your work on this "peer dependency" topic..
Thanks so far for your work

@IbrahimSulai
Copy link

@xstable - I have fixed the play-button issue and committed the same today in the above mentioned PR. If you needed you can try using patch-package . Thanks.

@ahartzog
Copy link
Author

ahartzog commented Jan 25, 2019

@xstable @IbrahimSulai I have a new PR opened with this:

#1095

Also, you can already pass a custom video component in. My PR updates the documentation to reflect this as well. I use react-native-video-controls as my control view on Android and it works fine.

@xstable
Copy link
Contributor

xstable commented Jan 28, 2019

@ahartzog for me it didn't work. Even if I've changed the dependency in gifted-chat's package.json to peer-dependency, the same error occur if I use gifted-chat > 0.5.0.
Any Idea why?

react-native-video-controls dependency is still set to react-native-video": "^2.0.0 and there seems no maintenance for a longer time, so I prefer to use the controls directly from react-native-video now, like IbrahimSulai implemented

@ahartzog forget my previous post. I've used patch-package but didn't notice, that it don't be able to patch the package.json.
So after I've changed the package.json like you described, and removed the ./node_modules/react-native-video Directory inside of react-native-gifted-chat Folder, it works.

@xcarpentier So hopefully it will be merged soon.

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.

5 participants