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

Support RN 0.39.2 and RN 0.40.0 #469

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@g3r4n
Copy link

g3r4n commented Jan 26, 2017

@mehcode

This comment has been minimized.

Copy link

mehcode commented Jan 27, 2017

@brentvatne @jhabdas

Can we please get this merged / released?

@jhabdas

This comment has been minimized.

Copy link
Collaborator

jhabdas commented Jan 28, 2017

@AndrewJack I saw a thumbs down. Any reason in particular? I can help get the changes merged and coordinate with @brentvatne on the release. But before I do I'd like to hear from at least one other person these changes are working well for them after a fresh RN init and module link.

@AndrewJack

This comment has been minimized.

Copy link
Contributor

AndrewJack commented Jan 28, 2017

@jhabdas I think it would be easier to not support older react native versions, given the frequency of releases (even if they are now monthly).

However since this library hasn't been published recently, I can see this change being useful. If we could release more often then we could avoid doing things like this.

@jhabdas

This comment has been minimized.

Copy link
Collaborator

jhabdas commented Jan 28, 2017

Thanks for the clarification @AndrewJack, and I agree. Pragmatically, if this supports more than one version I'll take it. Just need someone else to confirm it's working as expected on a fresh RN init with video linked.

@edmundito

This comment has been minimized.

Copy link

edmundito commented Jan 28, 2017

I'm happy either way, and I've been tracking other dependent modules that need to be updated to 0.40, and there's a general pattern that I'm ok with:

  1. Make changes without backward compatible support
  2. Bump the minor number
  3. Update the README to denote what version is compatible with 0.39 or below.
@g3r4n

This comment has been minimized.

Copy link
Author

g3r4n commented Jan 29, 2017

I was not aware that community didn't like backward compatible support. I understand that is more efficient to support new version of RN. I will work on that.

@g3r4n g3r4n closed this Jan 29, 2017

@jhabdas

This comment has been minimized.

Copy link
Collaborator

jhabdas commented Jan 30, 2017

Thanks @g3r4n. Sorry if you lost any time. I was okay with this PR so long as it could be verified functional with the latest version of RN. As for the community, and I'm sure I don't speak for everyone, it's unlikely to see much backwards compatibility being baked in modules unless they're extremely widely used or backed by funding like CodePush, at least until RN hits a major semantic version.

@g3r4n

This comment has been minimized.

Copy link
Author

g3r4n commented Jan 30, 2017

I'm new to open source so i have to learn. Thanks for the explanation !

@jhabdas

This comment has been minimized.

Copy link
Collaborator

jhabdas commented Jan 30, 2017

No problem. Your contributions are appreciated regardless of experience level. I'm going to reopen this to increase visibility. As mentioned earlier, I will merge these changes and work to get the NPM module updated as soon as someone can prove out the functionality with the latest version of RN. Thanks again, @g3r4n. 😃

@jhabdas jhabdas reopened this Jan 30, 2017

@g3r4n

This comment has been minimized.

Copy link
Author

g3r4n commented Feb 4, 2017

it seems that this PR will not be merge because the next release will only support RN >=0.40.0

@g3r4n g3r4n closed this Feb 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment