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

[V3] Update react-native-experimental-navigation to new version with PropTypes fix #2541

Conversation

daviscabral
Copy link
Collaborator

@daviscabral daviscabral commented Oct 24, 2017

This PR is dependent of aksonov/react-native-experimental-navigation#31 (where the version is updated).

Babel presets and plugins were also updated in the process.

@daviscabral daviscabral changed the title Move to reactxp-experimental-navigation and update babel presets/plugins [V3] Move to reactxp-experimental-navigation and update babel presets/plugins Oct 24, 2017
@daviscabral
Copy link
Collaborator Author

@aksonov any objection in going forward with this kind of update? That’s the maintenance of v3 that I would like to keep - that I told you about in #69.

@ujwal-setlur
Copy link

ujwal-setlur commented Oct 24, 2017

@daviscabral , don't you need to change the import statements to import reactxp-experimental-navigation instead of react-native-experimental-navigation? For example, in Router.js:

import NavigationExperimental from 'react-native-experimental-navigation';

I would imagine this import statement would have to change to reference reactxp-experimental-navigation.

@daviscabral
Copy link
Collaborator Author

@ujwal-setlur good catch! I was under the impression I had committed those changes. I'll get those committed and pushed now.

@schlaegerz
Copy link

I tried using this branch and it doesn't seem to work.
Router.js is doing the following:

const {
  RootContainer: NavigationRootContainer,
} = NavigationExperimental;

And the branch you are using doesn't have RootContainer defined as part of NavigationExperimental

@daviscabral
Copy link
Collaborator Author

@schlaegerz you are totally right. I know what happened - I've have in my app with a custom reducer and router - what made I miss it. My bad - I'll work in a proper fix. Probably worth to patch http://github.com/aksonov/react-native-experimental-navigation instead of moving over a totally different project.

@daviscabral daviscabral changed the title [V3] Move to reactxp-experimental-navigation and update babel presets/plugins [V3][WIP] Move to reactxp-experimental-navigation and update babel presets/plugins Oct 25, 2017
@daviscabral
Copy link
Collaborator Author

Working to fix this right now 👍

@daviscabral daviscabral changed the title [V3][WIP] Move to reactxp-experimental-navigation and update babel presets/plugins [V3] Update babel presets/plugins and react-native-experimental-navigation to new version with PropTypes fix Oct 25, 2017
@daviscabral daviscabral changed the title [V3] Update babel presets/plugins and react-native-experimental-navigation to new version with PropTypes fix [V3] Update react-native-experimental-navigation to new version with PropTypes fix Oct 25, 2017
@daviscabral
Copy link
Collaborator Author

I am with a new local branch working in a way to bring useNativeDriver to the animations - but it seems that is really broken. Not sure if worth the trouble. I'll keep cleaning up stuff to react 16.0.0.

@ujwal-setlur
Copy link

Let me know if you need any help. I really want to move to react-native 0.49 and react 16, and this issue is stopping me :)

@schlaegerz
Copy link

Thank you so much! After fixing the last of the PropTypes issues and using this branch I was finally able to get it all working again.

@daviscabral
Copy link
Collaborator Author

@schlaegerz glad to know. I changed my mind about using the MS fork - it seems they made a lot of changes there to use it with Skype. Not sure if the whole work to make it functional with v3 makes sense. An upgrade to v4 sounds more sensate.

@ujwal-setlur
Copy link

@daviscabral This is not working for me. I took the latest from your dc-use-react-native-experimental-navigation-from-ms branch. I still get the following error:

screen shot 2017-10-25 at 2 50 10 pm

Looking at the code, it seems you are still importing "PropTypes" from react, and not prop-types?

@daviscabral
Copy link
Collaborator Author

@ujwal-setlur try to remove your node_modules and clean your npm cache. I got the same issue here, and since we haven't bumped the version yet - it was still getting the same old package. Let me know if that does not work.

@schlaegerz
Copy link

There were some leftover propTypes, i branched this one myself, if you want to just test out a temporary branch https://github.com/schlaegerz/react-native-router-flux/tree/dc-use-react-native-experimental-navigation-from-ms

@daviscabral
Copy link
Collaborator Author

Good catch! My bad! 👍

@ujwal-setlur
Copy link

Yay, it works with this now! Thanks!

@ujwal-setlur
Copy link

@daviscabral it looks like react-native-experimental-navigation 0.28.0 uses React.PropTypes. That needs to be cleaned out as well. Bummer, so much cleanup work!

@aksonov
Copy link
Owner

aksonov commented Oct 26, 2017 via email

@ujwal-setlur
Copy link

@aksonov thanks! I just forked the repo to make the changes. Glad I don't have to!

@daviscabral can you update your branch to use 0.29.0?

@ujwal-setlur
Copy link

@aksonov there were duplicate require statements in two files. I have fixed that and raised a pull request: aksonov/react-native-experimental-navigation#32

@aksonov
Copy link
Owner

aksonov commented Oct 26, 2017 via email

@ujwal-setlur
Copy link

Yes it refuses to build because of the duplicate require statements

@aksonov
Copy link
Owner

aksonov commented Oct 26, 2017

Done, published 0.29.1

@ujwal-setlur
Copy link

Thanks!

@ujwal-setlur
Copy link

@aksonov thanks for meeting my PR. How about this PR from @daviscabral. I have tested it and it works. @daviscabral needs to update it to use 0.29.1 of react-native-experimental-navigation

@daviscabral
Copy link
Collaborator Author

I am not home now - but should be there soon to update it 👍🏻

@daviscabral daviscabral force-pushed the dc-use-react-native-experimental-navigation-from-ms branch from 762cda6 to 80e220a Compare October 27, 2017 00:48
@daviscabral
Copy link
Collaborator Author

Did some cleanup in the commits.

@aksonov Any chance of this getting merged?

@aksonov
Copy link
Owner

aksonov commented Oct 27, 2017

@daviscabral Could you set 0.29.1 as react-native-experimental-navigation ?

@daviscabral
Copy link
Collaborator Author

Yes - sorry - forgot to update it inside the Example folder.

@daviscabral
Copy link
Collaborator Author

@aksonov Done 👍🏻

@aksonov aksonov merged commit 5fa1565 into aksonov:v3 Oct 27, 2017
@daviscabral
Copy link
Collaborator Author

Thank you.

@ujwal-setlur
Copy link

Thanks @daviscabral and @aksonov. Will there be a new release?

Also, @aksonov, how hard is the migration from v3 to v4? I use switch which I believe is not there in v4?

@daviscabral
Copy link
Collaborator Author

@ujwal-setlur it's there already - 3.42.

@ujwal-setlur
Copy link

@daviscabral Oh, OK. I thought 3.42 was published before your merge.

@ujwal-setlur
Copy link

ujwal-setlur commented Oct 27, 2017

@daviscabral @aksonov I just checked and 3.42.0 does not have this pull request. In DefaultRenderer.js:

import React, {
  PureComponent,
  PropTypes,
} from 'react';

We need a 3.43.0 or 3.42.1

@aksonov
Copy link
Owner

aksonov commented Oct 27, 2017 via email

@ujwal-setlur
Copy link

That was quick 👍 Thanks!

@daviscabral daviscabral deleted the dc-use-react-native-experimental-navigation-from-ms branch December 4, 2017 01:51
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.

4 participants