Kureev / react-native-navbar

Navbar component for React Native
MIT License
1.89k stars 317 forks source link

RN 0.16.0-rc support? #116

Closed sankhadeeproy007 closed 8 years ago

sankhadeeproy007 commented 8 years ago

Am I the only one who hasn't got it to work with RN 0.16.0-rc? I keep getting this error. Any help?

screen shot 2015-12-02 at 7 16 09 pm

Kureev commented 8 years ago

Seems it needs to change stage in .babelrc as it described here

TL;DR:

We also added presets for stages such as babel-preset-stage-0 (used to be stage: 0 in .babelrc in babel 5.x).

chagasaway commented 8 years ago

Just upgraded react-native version here to 0.16.0 and got a similiar error.

TransformError: /app/node_modules/react-native-navbar/NavbarButton.js: [BABEL] /app/node_modules/react-native-navbar/NavbarButton.js: Unknown option: /app/node_modules/react-native-navbar/.babelrc.whitelist

Is there any workaround?

chetstone commented 8 years ago

I wonder if .babelrc could just be removed? Could be that RN packager by default now enables any transforms needed by react-native-navbar.

leemarreros commented 8 years ago

This is how I fixed it: Babel specifies that there is a specific preset for React. { "preset": "react" } Therefore, I changed .babelrc contained in react-native-navbar to this: { "retainLines": true, "compact": true, "presets": ["react"], // or "preset": "react" "comments": false, "sourceMaps": false } I am not using white list anymore because all its elements are now under presets.

Kureev commented 8 years ago

Should be fixed in 1.1.x, can TS verify and close the issue?

sankhadeeproy007 commented 8 years ago

Hi @Kureev, I tried 1.1.1, but I'm getting this error while build. I deleted my node_modules folder and tried again, but to no avail. Any help regarding this? screen shot 2015-12-14 at 1 16 34 pm

Kureev commented 8 years ago

Well, 1.1.1 assumes you're using npm3, so you have a flat dependency tree and the error above can't occur. If you're using previous version of npm and don't want to upgrade, you can fork previous version and remove .babelrc to make it work in npm2 and babel 6.

grabbou commented 8 years ago

Well, 1.1.1 assumes you're using npm3

Why such constraint? How come you can assume npm version? AFAIK there are no deal-breaking changes between them.

I would like to note that there's not even a single reference to this module from the stack trace above, so it's related to npm3 and react-native itself. I went back to npm2 because of the above (it does not make flat dependency tree in all cases, it only flattens when versions are matching as far as I know leaving other versions in original node_modules of a dependency which results in naming collision - had that with react-tools installed, react and react-native in one project (universal app).

Kureev commented 8 years ago

Why such constraint? How come you can assume npm version? AFAIK there are no deal-breaking changes between them.

Because as you can see from above, @sankhadeeproy007 has an issue having 2 react libraries in different folders which are conflicting with each other. Anybody can use previous versions for npm2, from 1.1 I'm using dependencies instead of peerDependencies, and that's why it installs 2 versions of RN with npm2.

I'm sorry for your experience with npm3, but that's how ecosystem moves forward. It's considered as a stable release, so I'll aim for it.

grabbou commented 8 years ago

Ha, to me - that's invalid https://github.com/react-native-fellowship/react-native-navbar/blob/master/package.json#L18.

Why it's listed in dependencies, not devDependencies nor peerDependencies ? That's what everyone's been doing (including your other repositories) and works great w/o forcing npm versions.

Also - with such structure, npm3 flat dependencies tree is not going to give you anything but errors if I have 0.15 installed - I will end up with two copies anyway (0.16 in your node_modules, 0.15 in project node_modules) + no warning will be printed about different versions.

And sorry, missed that there was react-native-navbar in one of the paths.

Kureev commented 8 years ago

Because devDependencies - dependencies for developing library itself. About peerDependencies:

We will also be changing the behavior of peerDependencies in npm@3. We won’t be automatically 
downloading the peer dependency anymore. Instead, we’ll warn you if the peer dependency isn’t already 
installed. This requires you to resolve peerDependency conflicts yourself, manually, but in the long run 
this should make it less likely that you’ll end up in a tricky spot with your packages’ dependencies.

So, according to the paragraph above, I consider the best place for react-native lib is inside dependencies section.

grabbou commented 8 years ago

That's actually a good thing and documentation clearly describes the desired use-case for that: https://docs.npmjs.com/files/package.json#peerdependencies

In some cases, you want to express the compatibility of your package with a host tool or library

As I said - listing that in dependencies assumes that react-native-navbar runs as a standalone module bundled with its own react-native version which is not true.

By moving it to dependencies you are just replicating deprecated peerDependencies behaviour (so-called autoinstalling). As already commented above, it introduces silent conflicts with versions and naming conflicts (it's also slower to install because of two different react-native versions to compile).

It's worth noting that the above set up is going to give duplicated versions almost all the time, why? Because for now, that plugin explicitly marks ^0.16 in dependencies. That means, for all users below 0.16 - it's not going to work (naming collision).

That is just wrong and reintroduces dependency hell again ;)

To me, it should stay as peerDependency because that's not only what everyone is doing, but also what peerDependencies has been made for. And with npm3 - it works even better, because it gives you nice warnings instead of stopping the entire installation process.

And I can't see community moving towards that, at least in rackt:

https://github.com/rackt/react-router/blob/master/package.json#L40-L41 https://github.com/rackt/react-redux/blob/master/package.json#L74-L75 https://github.com/rackt/redux-router/blob/master/package.json#L51-L54

Kureev commented 8 years ago

As already commented above, it introduces silent conflicts with versions and naming conflicts (it's also slower to install because of two different react-native versions to compile).

That's why I've added versions into requirements.

It's worth noting that the above set up is going to give duplicated versions almost all the time, why? Use a prev. version of navbar for npm2.

Because for now, that plugin explicitly marks ^0.16 in dependencies. That means, for all users below 0.16 - it's not going to work (naming collision)

They should use previous version. Old RN wouldn't work with babel 6.

Assuming everyone has upgraded and 0.17 is out - you bump it to 0.17 to make it working and then - it does not work for all the others that are on 0.16

Yeah, probably it should be >= 0.16 instead

grabbou commented 8 years ago

Because for now, that plugin explicitly marks ^0.16 in dependencies. That means, for all users below 0.16 - it's not going to work (naming collision)

This package didn't change anything to adjust to Babel 6 apart from removing babelrc as far as I am aware - it's going to work anyway because the same transformers were available anyway.

Previously, it was using peerDependencies to accomplish the same https://github.com/react-native-fellowship/react-native-navbar/blob/6ca34a9c9c51ea5bc8d4abd99a78952834e04385/package.json#L18. If there wasn't matching react version, there was a warning. Clear & obvious.

Now, what are the benefits of current way? If we had the warning - @sankhadeeproy007 wouldn't have submitted that issue/comment because of clear info about incompatible versions and there wouldn't be anything to discuss.

It's up to you - to me it's not an idiomatic way to handle dependencies in npm registry, because we assume the tree is always flattened and react-native is moved out from navbar dependencies to project dependencies (level up). The question is - when it does not happen, what's the purpose of react-native inside navbar node_modules? Because I can't see any.

The current approach works better for something like react-addons - if project uses version similar to ours - flatten, otherwise leave it as is. Since they can co-exists, there's no issue. And we, as a package authors should not rely on that since it's perf optimisation done during installation and should be completely transparent for us. For marking compatible versions, we should use features meant for that.

tangrufus commented 8 years ago

Same error on react native 0.17.0, even with .babelrc files removed.

Any work around?

Error building DependencyGraph:
 Error: Naming collision detected: /Users/me/Documents/react-native/myApp/node_modules/react-native/Libraries/vendor/react/platformImplementations/universal/worker/UniversalWorkerNodeHandle.js collides with /Users/me/Documents/react-native/myApp/node_modules/react-native-navbar/node_modules/react-native/Libraries/vendor/react/platformImplementations/universal/worker/UniversalWorkerNodeHandle.js
Kureev commented 8 years ago

Ok, that became annoying, so I just reverted dependencies back to peerDependencies. Released in 1.1.2

grabbou commented 8 years ago

See #127