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

Removed NavigationExperimental dependency #242

Closed
wants to merge 12 commits into from

Conversation

JulianKingman
Copy link

Fixes this issue: #241

RN 0.44.0 eliminated NavigationExperimental, so I copied their style for navigation bar height and put it into the default variables to make it overwritable.

Copy link

@aswinmohanme aswinmohanme left a comment

Choose a reason for hiding this comment

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

Shouldn't the variabls on the newly added lines actually be variables

@SoHotSoup
Copy link
Contributor

Thank you for submitting it, but this issue would persist for anyone using CardStack and NavigationBar from @shoutem/ui/navigation. The real solution would be to migrate them to use react-navigation, but I believe that this is too much to do for now. So I suggest to take the NavigationExperimental files from RN 0.42.0 and copy them here. It's little bit hacky, but it would fix all navigation issues on RN 0.44.0. What you say?

@SoHotSoup
Copy link
Contributor

#244

@JulianKingman
Copy link
Author

According to FB the views exist in react-navigation

NavigationExperimental is deprecated and will be removed in a future 
version of React Native. The NavigationExperimental views live on in 
the React-Navigation project, which also makes it easy to declare
navigation logic for your app. Learn more at https://reactnavigation.org/

@JulianKingman
Copy link
Author

I replaced navigationexperimental with react-navigation, is there a repo that I can test it with? I don't have any projects that use the header or cardstack.
Still todo: draw in the header height from the Header component like it was originally.

@SoHotSoup
Copy link
Contributor

@JulianKingman RestaurantsApp in examples uses it, so please try to run an app and verify that everything works...

@JulianKingman
Copy link
Author

JulianKingman commented May 10, 2017

Hm, this might take some work. They changed the API for the cardstack in the most recent version, I'm thinking of just switching to the new API. If I can get away with just fixing the current errors, I will (tracking here: react-navigation/react-navigation#1445)

@SoHotSoup
Copy link
Contributor

@JulianKingman Sorry, I was offline few days. Thank you for doing this issue, is there anything I can do to help you?

@davidvuong
Copy link

Hey guys, what's the status on this? I'm currently referencing the latest commit in this PR until this is merged. I'm under the impression that some components won't work even with the fix now? Is it possible for me to get a list of working components?

@JulianKingman
Copy link
Author

The problem are the navigation components, which rely on NavigationExperimental, which was removed from react in 0.44.0 (I think).
The crux of the issue is that the components were ported to react-navigation, but now they have a different API, so I'm not sure if it will be possible to make it fully backward compatible. @SoHotSoup what are your thoughts, is backwards compatibility a must? I think the main change is using react-navigation's built in navigation props, instead of passing in a redux state specifically.

@jcguarinpenaranda
Copy link

Please merge this!! I am using react-navigation and I am getting the error: undefined is not an object (evaluating '_reactNative.NavigationExperimental.Header')

@alxvallejo
Copy link

alxvallejo commented Jun 2, 2017

+1, I rely on this PR, but it is causing issues when I use a Tile component:

ExceptionsManager.js:71 Warning: Failed prop type: Invalid props.style key heroAnimation supplied to AnimatedComponent.

@tafelito
Copy link

tafelito commented Jun 7, 2017

+1, same as @alxvallejo I get the same warning

@SeptiyanAndika
Copy link

+1, same as @jcguarinpenaranda , i get that error. (evaluating '_reactNative.NavigationExperimental.Header')

@JulianKingman
Copy link
Author

Waiting on @SoHotSoup to decide what to do about backwards-compatibility of navigation components.

@ardavank
Copy link

Any update on this?

@Talor-A
Copy link

Talor-A commented Jun 13, 2017

to anyone having this issue try deleting ~/.npm/ and your node_modules/ folder. this was causing the wrong version of react-native to install for me.

@juji
Copy link

juji commented Jun 17, 2017

+1 Even the sample cannot be loaded...

@elopez89
Copy link

Any updates on this?

@JulianKingman
Copy link
Author

I'm looking into what's involved in using the updated API, assuming that works for @SoHotSoup. Unfortunately, there is no documentation yet (tracking here: react-navigation/react-navigation#1946), so it's kind of trial and error..

@JulianKingman
Copy link
Author

@SoHotSoup after looking more in depth into the various API's and changes, it seems like the most sensible solution will be to remove navigation from shoutem/ui. I updated my branch accordingly, as well as the restaurant app. The result is a significant simplification of the code.

For posterity, here are all the potential options I can think of:

  1. Create a polyfill for the way react-navigation deals with CardStack to match current shoutem/ui API. This would be fragile and likely to break in a new release.
  2. Copy relevant code from NavigationExperimental into shoutem/ui core. This seems like a dead end, but at least it would be stable.
  3. Take navigation out of shoutem/ui. This seems preferrable to me, as navigation is quite complex (even in the Shoutem implementation), react-navigation is the future, and it's relatively simple to use.

@eightyfive
Copy link

My humble opinion is to go for option 3. I don't think shoutem UI Toolkit should include any navigation logic, just UI. Pure UI framework. But this sounds unrealistic.

  1. A more viable solution is to migrate to react-navigation

@SoHotSoup Any plan to do so? Need help? What's the plan at Shoutem to deal with this issue? I guess this is actively being looked into, since you whole ecosystem cannot upgrade to new RN updates.

@ardavank
Copy link

I agree with @eightyfive and @JulianKingman, Shoutem UI should only contain the UI components and not the navigation or any other logics.

@AkdM
Copy link

AkdM commented Jul 12, 2017

While I totally agree about having only the UI and not the logic, would this mean that components like the DropdownMenu would get kicked out since there’s some logic in it?

@eightyfive
Copy link

@AkdM I don't think Shoutem will ever have a plan to ditch the navigation logic from the Toolkit. It would impact too much their existing product.

Let's wait for an input from someone working at Shoutem.

@AkdM
Copy link

AkdM commented Jul 12, 2017

@eightyfive My guess would be to have two different repos or even branches, one with the fixed Navigation and the other one without.

@eightyfive
Copy link

Yes, I would like that as well 😄. But unfortunately this is not in our hands.

@SoHotSoup
Copy link
Contributor

@eightyfive we have opted for 3rd solution and we are actively working on moving our main product to use react-nativation. I guess that would be finished and deployed in next few weeks, as part of that task we will remove these navigation components from our UI.

@AkdM
Copy link

AkdM commented Jul 12, 2017

Great news! Thanks for the update @SoHotSoup 👍

@pavitran
Copy link

@SoHotSoup any update on this?

@kiorq
Copy link

kiorq commented Aug 5, 2017

any update? i get an error just by importing. making it completely unusable

@leandrodragani
Copy link

what happen with this ?

@davidvuong
Copy link

Judging by this commit: 3bc0856

shoutem/ui now might be compatible with later versions of React Native which no longer have NavigationExperimental? I haven't had a chance to test it myself.

@Definitely-Not-Vlad
Copy link
Collaborator

We've resolved it with the commit davidvuong mentioned. I wanted to update the thread immediately but I didn't get a chance to test it out completely.

I can now run the <Examples /> without any of the previous workarounds (dropping RN version, manually importing NavigationExperimental, etc.).

Here's my package.json dependency versions:

{
  "name": "UITest",
  "version": "0.0.1",
  "private": true,
  "scripts": {
    "start": "node node_modules/react-native/local-cli/cli.js start",
    "test": "jest"
  },
  "dependencies": {
    "@shoutem/ui": "^0.21.0",
    "react": "16.0.0-alpha.12",
    "react-native": "0.47.1"
  },
  "devDependencies": {
    "babel-jest": "20.0.3",
    "babel-preset-react-native": "2.1.0",
    "jest": "20.0.4",
    "react-test-renderer": "16.0.0-alpha.12"
  },
  "jest": {
    "preset": "react-native"
  }
}

@JulianKingman
Copy link
Author

PR no longer necessary

@princenaman
Copy link

On a fresh react-native init
Still getting:

warning "@shoutem/ui@0.21.1" has incorrect peer dependency "react@^15.0.0".
warning "@shoutem/theme@0.10.0" has incorrect peer dependency "react@^15.1.0".
warning "@shoutem/animation@0.11.1" has incorrect peer dependency "react@^15.0.0".

package.json:

{
  "name": "sample",
  "version": "0.0.1",
  "private": true,
  "scripts": {
    "start": "node node_modules/react-native/local-cli/cli.js start",
    "test": "jest"
  },
  "dependencies": {
    "@shoutem/ui": "^0.21.0",
    "react": "16.0.0-alpha.12",
    "react-native": "0.47.1"
  },
  "devDependencies": {
    "babel-jest": "20.0.3",
    "babel-preset-react-native": "2.1.0",
    "jest": "20.0.4",
    "react-test-renderer": "16.0.0-alpha.12"
  },
  "jest": {
    "preset": "react-native"
  }
}

@bhosle-git
Copy link

Hi,

i'm facing the same issue - does the fix work with RN 0.44 also or do i have to upgrade to RN 0.47?

thanks

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.