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

[WIP] Allow react-native link <library> #485

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@tadeuzagallo
Contributor

tadeuzagallo commented Mar 30, 2015

I've added the link method to the cli, but this relies on a minimum info on the package.json of both, the target library and the current project.
The only piece of info necessary is:

{
  "react-native": {
    "xcodeproj": "ProjectName"
  }
}

I've added a sample anyways... The code looks pretty messy right now, I still have to remove the RegExps but would like some input. Also it doesn't handle any edge cases as is, just looked at the result prxproj file after linking a library and reproduced it.

/cc @vjeux @sahrens

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Mar 31, 2015

Contributor

I've also created a gist, with 3 files:

  • the initial file, that Xcode generates,
  • a clean file, the same file file, after parsed and stringified
  • the final file, with the libraries added

https://gist.github.com/tadeuzagallo/dd046e738f4c87edac4f

I also added support for a libraries key, tu support the use case we have now, that is the debugger, but it should be pretty straightforward to add frameworks next.

It also tries to guess a lot of names, that should work well specially for the native libraries.

Contributor

tadeuzagallo commented Mar 31, 2015

I've also created a gist, with 3 files:

  • the initial file, that Xcode generates,
  • a clean file, the same file file, after parsed and stringified
  • the final file, with the libraries added

https://gist.github.com/tadeuzagallo/dd046e738f4c87edac4f

I also added support for a libraries key, tu support the use case we have now, that is the debugger, but it should be pretty straightforward to add frameworks next.

It also tries to guess a lot of names, that should work well specially for the native libraries.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Mar 31, 2015

Collaborator

👍 this will be a very convenient feature! No comment on the code itself.

Collaborator

brentvatne commented Mar 31, 2015

👍 this will be a very convenient feature! No comment on the code itself.

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Mar 31, 2015

Contributor

See also #235

Contributor

frantic commented Mar 31, 2015

See also #235

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Mar 31, 2015

Collaborator

The work on #235 is going to pave the way for a React Native ecosystem where different packages can share the same dependencies. For example React Core will probably have a dependency on pop for POPAnimation support, and someone else might publish a (hypothetical) ReverberatingSound library that uses pop's spring dynamics. The two modules must share the same copy of pop for the app to compile, and they may also have different version requirements (ex: React Core might need pop >= 1.0.7 while ReverberatingSound might only need pop ~1.0), which #235 will address. As facebook publishes more mobile infrastructure and projects line up (ex: React Native on ComponentKit with pop) I believe this is the direction to look in.

Collaborator

ide commented Mar 31, 2015

The work on #235 is going to pave the way for a React Native ecosystem where different packages can share the same dependencies. For example React Core will probably have a dependency on pop for POPAnimation support, and someone else might publish a (hypothetical) ReverberatingSound library that uses pop's spring dynamics. The two modules must share the same copy of pop for the app to compile, and they may also have different version requirements (ex: React Core might need pop >= 1.0.7 while ReverberatingSound might only need pop ~1.0), which #235 will address. As facebook publishes more mobile infrastructure and projects line up (ex: React Native on ComponentKit with pop) I believe this is the direction to look in.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Mar 31, 2015

Contributor

Instead of inventing our own package.json schema, we might want to consider just using a BUCK file for the details, and package.json would just point to that.

You might also want to use the xcodeproj tool to modify the xcode files instead of regexes:

https://github.com/CocoaPods/Xcodeproj

On Mar 31, 2015, at 8:10 AM, Tadeu Zagallo notifications@github.com wrote:

I've also created a gist, with 3 files:

the initial file, that Xcode generates,
a clean file, the same file file, after parsed and stringified
the final file, with the libraries added
https://gist.github.com/tadeuzagallo/dd046e738f4c87edac4f

I also added support for a libraries key, tu support the use case we have now, that is the debugger, but it should be pretty straightforward to add frameworks next.

It also tries to guess a lot of names, that should work well specially for the native libraries.


Reply to this email directly or view it on GitHub.

Contributor

sahrens commented Mar 31, 2015

Instead of inventing our own package.json schema, we might want to consider just using a BUCK file for the details, and package.json would just point to that.

You might also want to use the xcodeproj tool to modify the xcode files instead of regexes:

https://github.com/CocoaPods/Xcodeproj

On Mar 31, 2015, at 8:10 AM, Tadeu Zagallo notifications@github.com wrote:

I've also created a gist, with 3 files:

the initial file, that Xcode generates,
a clean file, the same file file, after parsed and stringified
the final file, with the libraries added
https://gist.github.com/tadeuzagallo/dd046e738f4c87edac4f

I also added support for a libraries key, tu support the use case we have now, that is the debugger, but it should be pretty straightforward to add frameworks next.

It also tries to guess a lot of names, that should work well specially for the native libraries.


Reply to this email directly or view it on GitHub.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Mar 31, 2015

Contributor

Well, there some things to consider in it, first is that #235 makes a whole environment around CocoaPods, and I didn't think we had settled for it.

I do understand what are shared dependencies, and cocoapods does solve a lot more than these few lines of code, and I do not intend to create a node-pods. The thing is that it doesn't solve the problem for android and it depends on ruby, for that I do agree that BUCK is a better choice, but as far as I know it's not settled as well.

But for the sake of clarity, I didn't create a package manager, I wrote a couple lines of code that allow you to link projects via cli, it's not a fabulous module system, it's just so people that are not iOS developers don't have to keep dragging files around neither are required to use cocoapods and therefore ruby.

Instead of inventing our own package.json schema, we might want to consider just using a BUCK file for the details, and package.json would just point to that.

It doesn't rely on package.json right now for the internal libraries, it was an idea.

You might also want to use the xcodeproj tool to modify the xcode files instead of regexes

It doesn't use any regex, and it would introduce a dependency on ruby all over again.

Contributor

tadeuzagallo commented Mar 31, 2015

Well, there some things to consider in it, first is that #235 makes a whole environment around CocoaPods, and I didn't think we had settled for it.

I do understand what are shared dependencies, and cocoapods does solve a lot more than these few lines of code, and I do not intend to create a node-pods. The thing is that it doesn't solve the problem for android and it depends on ruby, for that I do agree that BUCK is a better choice, but as far as I know it's not settled as well.

But for the sake of clarity, I didn't create a package manager, I wrote a couple lines of code that allow you to link projects via cli, it's not a fabulous module system, it's just so people that are not iOS developers don't have to keep dragging files around neither are required to use cocoapods and therefore ruby.

Instead of inventing our own package.json schema, we might want to consider just using a BUCK file for the details, and package.json would just point to that.

It doesn't rely on package.json right now for the internal libraries, it was an idea.

You might also want to use the xcodeproj tool to modify the xcode files instead of regexes

It doesn't use any regex, and it would introduce a dependency on ruby all over again.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Mar 31, 2015

Contributor

Oh, and something that might not be clear, where I mention libraries and frameworks I mean core .dylibs and .frameworks.

Contributor

tadeuzagallo commented Mar 31, 2015

Oh, and something that might not be clear, where I mention libraries and frameworks I mean core .dylibs and .frameworks.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Apr 1, 2015

Contributor

Sorry, I should have looked at your code more closely - this looks pretty sweet and is nice that it doesn't rely on any complex deps. I think once we have this, it shouldn't be too hard to create a react-native install script (maybe called as npm post-install?) that scans dependencies and calls this under the hood to add all the necessary libs. It doesn't address the versioning issues @ide mentioned, but I think it's a reasonable step for now without taking the CocoaPods plunge.

Contributor

sahrens commented Apr 1, 2015

Sorry, I should have looked at your code more closely - this looks pretty sweet and is nice that it doesn't rely on any complex deps. I think once we have this, it shouldn't be too hard to create a react-native install script (maybe called as npm post-install?) that scans dependencies and calls this under the hood to add all the necessary libs. It doesn't address the versioning issues @ide mentioned, but I think it's a reasonable step for now without taking the CocoaPods plunge.

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Apr 1, 2015

Contributor

My main concern is that it will introduce more problems in the future with supporting this. However, if you guys think it's worth doing I'd suggest to change the name of the command to something more concrete, like link-xcode-project or link-native-project and have a few tests for it.

Contributor

frantic commented Apr 1, 2015

My main concern is that it will introduce more problems in the future with supporting this. However, if you guys think it's worth doing I'd suggest to change the name of the command to something more concrete, like link-xcode-project or link-native-project and have a few tests for it.

@@ -13,6 +15,7 @@ function printUsage() {
'',
'Commands:',
' start: starts the webserver',
' link: link the target library the current project',

This comment has been minimized.

@frantic

frantic Apr 1, 2015

Contributor

not sure I follow the description

@frantic

frantic Apr 1, 2015

Contributor

not sure I follow the description

@@ -42,6 +52,265 @@ function init(root, projectName) {
spawn(path.resolve(__dirname, 'init.sh'), [projectName], {stdio:'inherit'});
}
function link(libraryPath) {
libraryPath = path.relative(process.cwd(), path.resolve(process.cwd(), libraryPath));

This comment has been minimized.

@frantic

frantic Apr 1, 2015

Contributor

do you actually need process.cwd() in path.resolve?

@frantic

frantic Apr 1, 2015

Contributor

do you actually need process.cwd() in path.resolve?

var xcodeprojName = targetName + '.xcodeproj';
var staticLibraryPath = pkg['react-native'].staticLibrary || 'lib' + targetName + '.a';
var pbxprojPath = path.resolve(process.cwd(), libraryPath,
xcodeprojName, 'project.pbxproj')

This comment has been minimized.

@frantic

frantic Apr 1, 2015

Contributor

nit: not a big fan of this style of indentation. I know we don't have coding style published anywhere, but I don't think this formatting is common in our codebase. Also it makes harder to make changes and maintain blame.

@frantic

frantic Apr 1, 2015

Contributor

nit: not a big fan of this style of indentation. I know we don't have coding style published anywhere, but I don't think this formatting is common in our codebase. Also it makes harder to make changes and maintain blame.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Apr 1, 2015

Contributor

Honestly, I don't feel comfortable landing it, but thanks for reviewing.

Contributor

tadeuzagallo commented Apr 1, 2015

Honestly, I don't feel comfortable landing it, but thanks for reviewing.

} catch(err) {
pkg = { 'react-native': {} };
}
var name = pkg['react-native'].xcodeproj || path.basename(libraryPath);

This comment has been minimized.

@frantic

frantic Apr 28, 2015

Contributor

if package.json exists but doesn't contain react-native, this will fail

@frantic

frantic Apr 28, 2015

Contributor

if package.json exists but doesn't contain react-native, this will fail

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Apr 28, 2015

Contributor

Few ideas for tests, if you decide to add them:

  • End to end style - call react-native link on sample project (see e2e-test.sh)
  • Extract function that deals with Xcode projects and test it on a few tiny Xcode apps (not necessarily RN)
Contributor

frantic commented Apr 28, 2015

Few ideas for tests, if you decide to add them:

  • End to end style - call react-native link on sample project (see e2e-test.sh)
  • Extract function that deals with Xcode projects and test it on a few tiny Xcode apps (not necessarily RN)
@@ -31,6 +34,13 @@ function run() {
process.cwd(),
], {stdio: 'inherit'});
break;
case 'link':
if (!args[1]) {
console.error('Usage: react-native link <path>');

This comment has been minimized.

@frantic

frantic Apr 28, 2015

Contributor

from the description it's not clear how to use it. I think the most logical use case is something like react-native link react-native-video

@frantic

frantic Apr 28, 2015

Contributor

from the description it's not clear how to use it. I think the most logical use case is something like react-native link react-native-video

This comment has been minimized.

@tadeuzagallo

tadeuzagallo Apr 28, 2015

Contributor

I'm thinking about changing the signature something more like react-native link MyProject[.xcodeproj] path/to/lib[.xcodeproj] so it doesn't rely on any magic, and if we decide to something easier, we can build on top of it.

@tadeuzagallo

tadeuzagallo Apr 28, 2015

Contributor

I'm thinking about changing the signature something more like react-native link MyProject[.xcodeproj] path/to/lib[.xcodeproj] so it doesn't rely on any magic, and if we decide to something easier, we can build on top of it.

This comment has been minimized.

@brentvatne

brentvatne Apr 28, 2015

Collaborator

@tadeuzagallo - good idea, the ideas are certainly not mutually exclusive - if we later add support for @frantic's proposed api (which looks ideal certainly) we can just switch between the two depending on the arity. I can imagine a number of edges cases that would get in the way of react-native link react-native-video, which would require more careful consideration. It's not a big hassle to include in the README for a project npm i x --save && react-native link MyProject node_modules/x/X

@brentvatne

brentvatne Apr 28, 2015

Collaborator

@tadeuzagallo - good idea, the ideas are certainly not mutually exclusive - if we later add support for @frantic's proposed api (which looks ideal certainly) we can just switch between the two depending on the arity. I can imagine a number of edges cases that would get in the way of react-native link react-native-video, which would require more careful consideration. It's not a big hassle to include in the README for a project npm i x --save && react-native link MyProject node_modules/x/X

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne May 15, 2015

Collaborator

@tadeuzagallo - any update here?

Collaborator

brentvatne commented May 15, 2015

@tadeuzagallo - any update here?

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo May 20, 2015

Contributor

@brentvatne Not yet. I'm on holidays and we'll be back next week. Feel free to jump in if you feel like helping!

Contributor

tadeuzagallo commented May 20, 2015

@brentvatne Not yet. I'm on holidays and we'll be back next week. Feel free to jump in if you feel like helping!

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne May 22, 2015

Collaborator

@tadeuzagallo - oh sorry for bothering you! Enjoy the well-deserved holidays 🍻 🌴!

Collaborator

brentvatne commented May 22, 2015

@tadeuzagallo - oh sorry for bothering you! Enjoy the well-deserved holidays 🍻 🌴!

@jtremback

This comment has been minimized.

Show comment
Hide comment
@jtremback

jtremback May 30, 2015

Contributor

👍

Contributor

jtremback commented May 30, 2015

👍

@jordanbyron

This comment has been minimized.

Show comment
Hide comment
@jordanbyron

jordanbyron Jun 11, 2015

Contributor

Hi @tadeuzagallo!

I was interested in pushing this forward and wanted to get your take on my path forward 😄

  1. Pull all of the kick ass code you write to open up and modify .xcodeproj files and stash it in a dedicated project (xclink maybe?) independent of react-native. That way we can write some nice tests and the fine maintainers of this project don't have to take on the additional burden of maintaining that code. I'd imagine xclink wouldn't rely on any magic and expect target and source paths to specific XCode projects.

    xclink MyProject[.xcodeproj] path/to/lib[.xcodeproj]
    
  2. Finalize the api for react-native's link method and consider restoring some of that link react-native-video magic.

  3. Add an end-to-end test for the cli just to make sure our magic works. Since we'll already have plenty of tests for xclink these can get away with being pretty minimal.

  4. 💸

So before I dive in head first, what do you think? 😟

cc/ @brentvatne

Contributor

jordanbyron commented Jun 11, 2015

Hi @tadeuzagallo!

I was interested in pushing this forward and wanted to get your take on my path forward 😄

  1. Pull all of the kick ass code you write to open up and modify .xcodeproj files and stash it in a dedicated project (xclink maybe?) independent of react-native. That way we can write some nice tests and the fine maintainers of this project don't have to take on the additional burden of maintaining that code. I'd imagine xclink wouldn't rely on any magic and expect target and source paths to specific XCode projects.

    xclink MyProject[.xcodeproj] path/to/lib[.xcodeproj]
    
  2. Finalize the api for react-native's link method and consider restoring some of that link react-native-video magic.

  3. Add an end-to-end test for the cli just to make sure our magic works. Since we'll already have plenty of tests for xclink these can get away with being pretty minimal.

  4. 💸

So before I dive in head first, what do you think? 😟

cc/ @brentvatne

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Jun 11, 2015

Contributor

Hey @jordanbyron, it's great that you want to tackle it, I really haven't had any time to get to it lately :/

We cannot simply move this code into another project for now, but we can move it into a separate folder inside cli, that may also contain the tests. Later on we can think about that.

What really stopped me in first place is that "linking two projects" is a very vague thing, and we either end up assuming basically everything or we need some kind of configuration file, then we get back to the point of kinda creating a new package manager, that is totally not the idea.

Something that also has to be figured out is that currently we don't do any validations, things are just added to project, even if it's already there. But this one should be trivial, even if you have any issues I can fix it.

So, totally go for it, just give it a little bit of thought about what "link" in this context is going to mean: Can we specify a target? What if it's not a static library? What if it has a different name? Right now I add the headers path of the library being linked to the container library, but the library being linked will probably have to find React's headers as well. Just to be clear, we don't have to support everything, but it has to, at least, be flexible enough to handle the most common/basic cases.

And let me know if you need any help with the pbxproj tweaking code 😃

Contributor

tadeuzagallo commented Jun 11, 2015

Hey @jordanbyron, it's great that you want to tackle it, I really haven't had any time to get to it lately :/

We cannot simply move this code into another project for now, but we can move it into a separate folder inside cli, that may also contain the tests. Later on we can think about that.

What really stopped me in first place is that "linking two projects" is a very vague thing, and we either end up assuming basically everything or we need some kind of configuration file, then we get back to the point of kinda creating a new package manager, that is totally not the idea.

Something that also has to be figured out is that currently we don't do any validations, things are just added to project, even if it's already there. But this one should be trivial, even if you have any issues I can fix it.

So, totally go for it, just give it a little bit of thought about what "link" in this context is going to mean: Can we specify a target? What if it's not a static library? What if it has a different name? Right now I add the headers path of the library being linked to the container library, but the library being linked will probably have to find React's headers as well. Just to be clear, we don't have to support everything, but it has to, at least, be flexible enough to handle the most common/basic cases.

And let me know if you need any help with the pbxproj tweaking code 😃

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 20, 2015

@tadeuzagallo updated the pull request.

facebook-github-bot commented Oct 20, 2015

@tadeuzagallo updated the pull request.

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