General project refactor

piranna commented 6 years ago

This pull-request acomplish several things:

Checked and ensured re-base template generates a project that works untouched on Android, web and Electron (should work on iOS, but I can't be able to test it)
Removed useless or duplicated files from re-base template to make it as minimal and out-of-the-way as possible
Fixed templates placeholders so project name gets correctly filled everywhere
Added build scripts for all platforms to package.json, so there's no need to learn external tools, everything can be build with npm run <platform> and npm run <platform>:release.
Layered architecture so several templates can be applied ones on top of anothers to customize the scheleton created by re-base template (still untested, but should work)

amoghbanta commented 6 years ago

@piranna, great work man, might take me a couple of days to go through it and test it completely. I'll try to review and test it asap.

piranna commented 6 years ago

@piranna, great work man, might take me a couple of days to go through it and test it completely. I'll try to review and test it asap.

Don't worry, take your time :-) I'm doing some work towards having automated unit tests, so we could enable Greenkeeper again. Feel free to comment my commits :-P

piranna commented 6 years ago

Need to add test scripts for ios/android and release script for iOS.

I think I can add a release script for iOS, unluckily I don't have an OSX machine to test it. Electron would need some packager for the release version, but there are three projects for that and I'm not sure what to use...

What about addid a table to the Readme file about what are the current suported platforms and their status (debug only, release...)?

miguelocarvajal commented 6 years ago

@piranna does this mean you need help testing iOS?

I would be willing to help with this. I don’t use Electron and am not familiar with it but can certainly help with the iOS side. Just let me know what you need.

piranna commented 6 years ago

Electron is mostly done, can run it flawlessly on Linux and also can be able to generate releases for all available platforms... or at least the generate zip files automatically and distribute them :-P Would be good to generate platform specific packages (.deb, .rpm, .msi, .dmg...). If you can be able to test them in mac, it would be only to exec npm run electron.

I don't have an Apple computer so can't be able to test on iOS, but it would only be npm run ios and try to improve the automatiin process. I'm having some problems lately on Android and Windows with BundleBindings, seems due to some update, so probably they would happen on iOS too. We need to do automated tests to fix that things.

miguelocarvajal commented 6 years ago

Gave this a shot but got the following error:

error This module isn't specified in a manifest.
Failed to clean up template temp files in node_modules/re-route. This is not a critical error, you can work on your app.
fs.js:941
  binding.lstat(pathModule._makeLong(path));
          ^

Error: ENOENT: no such file or directory, lstat '/Users/omar/Temporary/templatetest/node_modules/re-route'
    at Object.fs.lstatSync (fs.js:941:11)
    at Object.lstatSync (/Users/omar/Temporary/templatetest/node_modules/graceful-fs/polyfills.js:297:22)
    at walk (/Users/omar/Temporary/templatetest/node_modules/react-native/local-cli/util/walk.js:13:11)
    at copyProjectTemplateAndReplace (/Users/omar/Temporary/templatetest/node_modules/react-native/local-cli/generator/copyProjectTemplateAndReplace.js:36:3)
    at createFromRemoteTemplate (/Users/omar/Temporary/templatetest/node_modules/react-native/local-cli/generator/templates.js:133:5)
    at createProjectFromTemplate (/Users/omar/Temporary/templatetest/node_modules/react-native/local-cli/generator/templates.js:86:5)
    at generateProject (/Users/omar/Temporary/templatetest/node_modules/react-native/local-cli/init/init.js:78:3)
    at Object.init (/Users/omar/Temporary/templatetest/node_modules/react-native/local-cli/init/init.js:50:5)
    at run (/Users/omar/.config/yarn/global/node_modules/react-native-cli/index.js:302:7)
    at createProject (/Users/omar/.config/yarn/global/node_modules/react-native-cli/index.js:249:3)

I tried running npm run electron anyway, but got this message:

npm ERR! missing script: electron

I might be able to look into this, do you have any insight as to what it might be to get me started?

Thanks!

piranna commented 6 years ago

Have you executed the ./finishInstall script?

miguelocarvajal commented 6 years ago

That script is not in the directory that react-native init creates.

Should I run it directly from the re-base directory?

piranna commented 6 years ago

That script is included in this pull-request template, if you are using it you should get it. How are you installing the template?

miguelocarvajal commented 6 years ago

I did see it included in the pull-request template, however its not in the resulting directory after I run this command:

react-native init templatetest --template file:///Users/omar/Downloads/re-start/templates/re-route

piranna commented 6 years ago

I got problems with using the templates from a file:// scheme, has you been able to make them work? Also, you can't be able to use directly re-route, the way I've done the pull-request, you need first to install re-base and later install re-route again on top of it, but I was not able to test this before. Can you be able to give it a try and tell me your results?

miguelocarvajal commented 6 years ago

If you are not using file:// then how are you doing the installation?

piranna commented 6 years ago

I uploaded my changes to npm with a renamed package, it's the only way I got it to work. You have more details in my blog.

miguelocarvajal commented 6 years ago

Ok so I got the template working.

Electron did work, the window opened with the debugger to the right:

iOS however, gave this error in the simulator:

Any idea as to what I can look for to solve the problem?

piranna commented 6 years ago

Electron did work, the window opened with the debugger to the right:

Yeah, I'm not sure what would be the best default, having de debugger open or clossed... Maybe clossed to have the same behaviour of other platforms?

iOS however, gave this error in the simulator:

The same BatchedBridge problem is hapenning in Android and Windows, seems there was a problem with one of the updates of ReactNative because re-start was working correctly and failed from one day to the next, but was not able to find a solution in Internet. It's not a problem of re-start, it's a problem of ReactNative, so here there's too few can do, I believe... :-/ Any help in this area is greatly welcome.

miguelocarvajal commented 6 years ago

Yeah, I'm not sure what would be the best default, having de debugger open or clossed... Maybe clossed to have the same behaviour of other platforms?

My vote is to have it default to the same as the other platforms. I guess for folks using Electron, they should know how to open the debugger or at it should be easy to find the instructions to do so.

The same BatchedBridge problem is hapenning in Android and Windows, seems there was a problem with one of the updates of ReactNative because re-start was working correctly and failed from one day to the next, but was not able to find a solution in Internet. It's not a problem of re-start, it's a problem of ReactNative, so here there's too few can do, I believe... :-/ Any help in this area is greatly welcome.

De madre! I do see it sparsely reported from a brief search. I'll research this further and post back anything I find. For now... a dormir!

miguelocarvajal commented 6 years ago

Quick update to the tests I've done.

I found that if you remove

  "plugins": [
    "react-native-web"
  ]

from the .babelrc file, the react-native run-ios command starts working again.

From briefly looking at the react-native-web documentation, they recommend putting these lines in the webpack config. I'm assuming so that it doesn't get included by Metro Bundler.

Any thoughts on this?

piranna commented 6 years ago

from the .babelrc file, the react-native run-ios command starts working again.

Have you tried if including them again, the problem still happens? babel-plugin-react-native-web replace the imports of react-native to directly reference the ones of react-native-web to reduce the generated bundle. If so, it would make sense to include it in the webpack config so only it's used for web and electron platforms...

piranna commented 6 years ago

Another alternative, maybe better, is to config the plugin in the babel configuration to only be executed for web platform...

miguelocarvajal commented 6 years ago

I definitely did try, I can easily reproduce / "fix" the problem by adding / removing the config from .babelrc. This should be on Linux / Android, can you reproduce as well?

Not sure how to apply babel config only to web and electron. Do you have any ideas?

piranna commented 6 years ago

I was thinking something like https://babeljs.io/docs/usage/babelrc/#env-option, but maybe https://github.com/facebook/metro/issues/47#issuecomment-346472714 could be more related.

piranna commented 6 years ago

Ok, seems the best way will be, as you suggested, to define it directly in the webpack config as it's suggested at https://github.com/necolas/react-native-web/blob/master/website/guides/getting-started.md#web-packaging-for-existing-react-native-apps:

{
    loader: 'babel-loader',
    options: {
      cacheDirectory: true,
      // The 'react-native' preset is recommended to match React Native's packager
      presets: ['react-native'],
      // Re-write paths to import only the modules needed by the app
      plugins: ['react-native-web']
    }
  }

Problem with that is that since internally react-scripts is being used, then we'll need to do a bit more monkey-patching if the scripts... Al least it's easy, but it's an ugly hack anyway :-/

miguelocarvajal commented 6 years ago

Gah.. that's what I was afraid of. Does that mean we'll have to eject?

Babel 7 does have better support with .babelrc.js (meaning we'll be able to check platform to set the plugin). However that also still has some pitfalls: https://github.com/bvaughn/react-virtualized/issues/427#issuecomment-393999056

piranna commented 6 years ago

Gah.. that's what I was afraid of. Does that mean we'll have to eject?

We could... but we don't must ;-) As I said, doing monkey-patching works, I have already done it to add support for user custom .babelrc files, take a look at the scripts/install.js script.

miguelocarvajal commented 6 years ago

Ah yes! I remember seeing that, I see what you mean now. That'll do it.

Don't want to duplicate the effort, do you prefer I do it or are you taking a stab at it?

piranna commented 6 years ago

Don't want to duplicate the effort, do you prefer I do it or are you taking a stab at it?

All for you :-) I'm a bit busy with other things, that's why I didn't give any time lately to this, so you are very welcome to help to improve this :-) Main things to work on are

[ ] fix build of untouched code now that we know it's not a ReactNative issue (thanks for find it! :-D )
[ ] scripts to automate release builds for iOS and Windows
[ ] check that layered templates works
[ ] automated tests

Maybe some of the things can be done in other pull-requests...

piranna commented 6 years ago

Any update @miguelocarvajal? Any stopper?

miguelocarvajal commented 6 years ago

No, sorry I just got caught up on some stuff here. Will make some time to work on this on the weekend!

piranna commented 6 years ago

Ok, keep going :-)

miguelocarvajal commented 6 years ago

Ok so far I have just the first item done:

[X] fix build of untouched code now that we know it's not a ReactNative issue (thanks for find it! :-D )
[ ] scripts to automate release builds for iOS and Windows
[ ] check that layered templates works
[ ] automated tests

Not really sure how to go about doing the scripts to automate release for iOS, seems pretty involved to script because AppDelegate needs to be modified, etc.

piranna commented 6 years ago

I have put some comments in your pull-requests, should not be difficult to fix :-)

Not really sure how to go about doing the scripts to automate release for iOS, seems pretty involved to script because AppDelegate needs to be modified, etc.

Nor do I. Maybe it could be relegated to other pull-request by someone else that has a Mac computer. Could you be able to check the layered templates mechanism?

miguelocarvajal commented 6 years ago

I didn't see the layered templates work correctly.

First I installed the re-base template, then the re-route template but the config in package.json was gone.

How are you installing it?

piranna commented 6 years ago

How are you installing it?

I didn't have time to test it yet myself, you are being currently my guinea pig in this feature :-P

piranna commented 5 years ago

Ok, so thanks to a current client interested on re-start, I've have time to review the refactor and finish it. The latest changes that I've done has been removal of setting devDepedencies since React Native CLI now has support for it, updated to Electron 4, and more important, I've been able to test and fix the layered templates, so now they are working correctly and we can start to create templates adjusted to our own needs. I could be able to merge it in master branch a version 0.4 but I don't have permissions to publish it on npm, so @amoghbanta, could you be able to take a look at the pull-request and publish it on npm? :-) By the moment they are published by myself as re-base_piranna and re-dux_piranna, but i would like to have it merged upstream...

react-everywhere / re-start

General project refactor #60