Leave only desktop-related code in repository

[vkjr]

The current repository has all files from facebook's react-native, plus files related to desktop implementation. On every merge with a new react-native update, we have a lot of conflicts to resolve due to this. Microsoft's react-native has a cleaner repository that contains only their code - https://github.com/Microsoft/react-native-windows

As far as I understand, code from react-native-windows repo applies on top of original react-native and extends functionality by adding a new platform support. End users shouldn't lose the possibility to develop mobile apps in the same repository as desktop ones.

We should clean up our repository to achieve the same result.

New changes in react-native should be helpful - https://facebook.github.io/react-native/docs/out-of-tree-platforms

[madhavarshney]

I've started working on this at https://github.com/madhavarshney/react-native-desktop/tree/RefactorRepo; I'll open an initial WIP PR tomorrow.

[vkjr]

@madhavarshney, thanks!

There is one case I'd like to mention. Lets say in some Foo.js file we previously added section like this:

if(platform.OS==="ubuntu") {
   doSomething();
}

In cleaned up code this if clause should be removed from Foo.js and instead Foo.desktop.js should be added with this clause.

[maxhora]

Hey @vkjr , @madhavarshney

Last time we have discussed the possibility to leave only desktop related code, we have decided to left Android and iOS related code too with hope that it will be possible to create desktop and mobile projects from one repo. (In past I've already removed most of not desktop related code, but later we decided to restore Android and iOS sources). Simplifying of merging is good argument, but final users seems will not be able to create iOS and Android apps from our repo.

Thanks

[vkjr]

@MaxRis, @madhavarshney, I didn't investigate how react-native-windows repo works in details but I was expecting that code from their repo extends original react-native with the new platform. I totally agree with @maxris that after cleanup in our repo we shouldn't lose the possibility to develop mobile apps.

@madhavarshney, do you understand how react-native-windows organized to patch react-native without losing mobile support?

[vkjr]

@madhavarshney, I edited the description of the issue to make it more clear that we don't wont to lose mobile features.

[madhavarshney]

@vkjr @MaxRis - Oh yes of course, I have that in mind. My work is still WIP so it's not going to be like that at the end. As per my understanding, what react-native-windows (and react-native-dom) does is it adds a new package for the platform-specific code/bridge, code generator, etc. and it adds an rnpm-plugin-platform for adding the platform to an existing react-native project created by react-native init. The plugin adds the command react-native platform which, when invoked, installs the react-native-platform "extension" and passes control to it. I haven't been able to this part working yet, however, but I'll investigate.

[madhavarshney]

@vkjr @MaxRis - Do you guys have plans of publishing monthly builds/releases of react-native-desktop to NPM instead of using GitHub to install the package? Once modularizing this repository is done (converting it to an extension of react-native), then you might want to publish react-native-desktop and rnpm-plugin-desktop. I noticed that react-native-desktop was published last year as react-native-qt, but there haven't been any more updates after that. You may want to contact @ptmt to see if he is willing to give the react-native-desktop package name as he discontinued the cross-platform react-native port he was working on and now solely focuses on MacOS.

[madhavarshney]

Also, do you use Docker and Jest? Seems like there are only android tests configured with docker, so I'm assuming you haven't setup docker tests desktop. I'm asking to make sure its safe to remove these configurations / files / directories.

[maxhora]

@madhavarshney we're running tests with Jest on CircleCI, but Docker is not used at all.

[maxhora]

@madhavarshney

Do you guys have plans of publishing monthly builds/releases of react-native-desktop to NPM instead of using GitHub to install the package?

That's definitely should happen, but we haven't planned specific milestone to do this. Thank you for your investigation regarding how windows port organized. That sounds like a plan! :)

[madhavarshney]

@vkjr @MaxRis - I noticed that when running react-native desktop, .bat or .sh scripts are generated based on the current platform. This means that a project bootstrapped on windows can only be run on windows and a project bootstrapped on MacOS/Linux can only run on MacOS/Linux. Is this by design? Also, I noticed that .flowconfig is slightly modified. Are these changes necessary? Diff:

diff --git a/.flowconfig b/.flowconfig
index 0b611f0..248daf9 100644
--- a/.flowconfig
+++ b/.flowconfig
@@ -2,6 +2,12 @@
 ; We fork some components by platform
 .*/*[.]android.js

+; Ignore templates for 'react-native init'
+.*/local-cli/templates/.*
+
+; Ignore the Dangerfile
+<PROJECT_ROOT>/danger/dangerfile.js
+
 ; Ignore "BUCK" generated dirs
 <PROJECT_ROOT>/\.buckd/

@@ -23,8 +29,9 @@

 [libs]
 node_modules/react-native/Libraries/react-native/react-native-interface.js
-node_modules/react-native/flow/
-node_modules/react-native/flow-github/
+node_modules/react-native/flow
+flow/
+flow-github/

 [options]
 emoji=true
@@ -35,18 +42,13 @@ munge_underscores=true

 module.name_mapper='^[./a-zA-Z0-9$_-]+\.\(bmp\|gif\|jpg\|jpeg\|png\|psd\|svg\|webp\|m4v\|mov\|mp4\|mpeg\|mpg\|webm\|aac\|aiff\|caf\|m4a\|mp3\|wav\|html\|pdf\)$' -> 'RelativeImageStub'

-module.file_ext=.js
-module.file_ext=.jsx
-module.file_ext=.json
-module.file_ext=.native.js
-
 suppress_type=$FlowIssue
 suppress_type=$FlowFixMe
 suppress_type=$FlowFixMeProps
 suppress_type=$FlowFixMeState

-suppress_comment=\\(.\\|\n\\)*\\$FlowFixMe\\($\\|[^(]\\|(\\(<VERSION>\\)? *\\(site=[a-z,_]*react_native[a-z,_]*\\)?)\\)
-suppress_comment=\\(.\\|\n\\)*\\$FlowIssue\\((\\(<VERSION>\\)? *\\(site=[a-z,_]*react_native[a-z,_]*\\)?)\\)?:? #[0-9]+
+suppress_comment=\\(.\\|\n\\)*\\$FlowFixMe\\($\\|[^(]\\|(\\(<VERSION>\\)? *\\(site=[a-z,_]*[react_native_oss|react_native_fb][a-z,_]*\\)?)\\)
+suppress_comment=\\(.\\|\n\\)*\\$FlowIssue\\((\\(<VERSION>\\)? *\\(site=[a-z,_]*[react_native_oss|react_native_fb][a-z,_]*\\)?)\\)?:? #[0-9]+
 suppress_comment=\\(.\\|\n\\)*\\$FlowFixedInNextDeploy
 suppress_comment=\\(.\\|\n\\)*\\$FlowExpectedError

[maxhora]

Hey @madhavarshney , it seems you are right, it doesn't have any sense to generate template with platform specific files. Both bat and sh files copying should be done.

Regarding .flowconfig file my best guess is that these changes are not required and were made automatically, but accidentally committed into git.

[maxhora]

Hey @madhavarshney , I'm going to start merging of newer react-native version later today, since we need it ASAP. I'm wondering, if you have started to do some work on this Issue, could you please just prepare PR with what you have currently, because merging will introduce much more conflicts to solve between new PRs. Thanks

[madhavarshney]

@MaxRis - Sure, I'll do that. I'm currently 70% done refactoring, but I haven't thoroughly tested the changes. If you're going to merge react-native onto my changes, it might be a little tough to see if my changes broke something or merging did so.

[maxhora]

@madhavarshney , thank you, if you're going to publish PR with changes, I will able to test it before merge against our custom app to check if something broken or not

[madhavarshney]

@MaxRis - Was this line added accidentally? It creates an extra ubuntu-server.js three directories above its own location, so when react-native-desktop is installed in node_modules, it produces an extra file outside of the root folder of a react-native-desktop project.

I came across this while modifying the build scripts to build in RNDesktopTest/desktop/build instead of building in RNDesktopTest/desktop so it is easier to ignore the build folder in git and delete the built output. Is this a good idea?

[maxhora]

Hey @madhavarshney , yes, that ubuntu-server.js file is not needed there, probably, have added this line by mistake. RNDesktopTest/desktop/build build folder should be good idea. But sometimes we're using heavy 3rd party libraries to build from sources as CMAKE external project ( like JavaScriptCore libs from WebKit ), it seems better to not delete built binaries every time for such externals ( with current implementation it worked fine and I've seen only first time run build of such dependencies, but not sure how will it be affected, if build folder will be used)

[madhavarshney]

@MaxRis - I'm not touching the way builds/rebuilds work, I'm just modifying the scripts to build in a subdirectory. In other words, build is not going to be deleted every rebuild, but nor is going to be checked in to git. (Keeping all the built files in the build directory allows easy .gitignoreing the folder, as react-native init already ignores build folders.)

[maxhora]

@madhavarshney , agree with that. I meant that clearing of build folder content on each run of react-native run-desktop command will not work good for some projects.