Skip to content
This repository has been archived by the owner on Jan 13, 2023. It is now read-only.

Update Ignite Bowser to RN 0.57.3 #103

Merged
merged 27 commits into from
Oct 12, 2018
Merged

Update Ignite Bowser to RN 0.57.3 #103

merged 27 commits into from
Oct 12, 2018

Conversation

nirre7
Copy link
Contributor

@nirre7 nirre7 commented Sep 25, 2018

Hi.

I've updated Ignite Bowser to RN 0.57.1 and bumped multiple dependencies.
Babel 7 has been the major thing as you will se in the commits.
Android seems to be working.
IOS is almost there just a couple outstanding issues.

Outstanding issues (that I need help with before you can merge this)

  • The SplashPagePatch. Currently the IOS build breaks "SplashScreen.h' file not found" which I think is caused by the splashPagePatch file needs to be updated to patch the files from RN 0.57, i.e it needs to be updated with the correct "hunks".✅
  • The jest setup in package.json is temporary. I got it to work with babel and ts but we probably want to "package" it a bit better alt update "jest-preset-ignite" @skellock && @marcelkalveram thanks! ✅
  • Do we still need the react-native+0.55.12.patch?
  • Trailing comma or not, in text-field.tsx? TS Lint gets angry or compiler gets angry … Allow trailing commas after non-rest elements in destructuring microsoft/TypeScript#24672

Would be great to get this merged with Bowser since I've got a big project coming up ;)

Thanks in advance
Niclas

@marcelkalveram
Copy link
Contributor

@nirre7 great work on the update. I was attempting the same with RN 0.56 but didn't get to submit a PR yet. However I can help out with a few issues.

  1. As for the splash screen patch, use the fix that @bryanstearns kindly provided in my attempt to upgrade to 0.56: marcelkalveram@aca8745

  2. I had to revert the jest config in package.json to the following to make it work:

  "jest": {
    "preset": "jest-preset-ignite"
  },

Looks like the lib received a typescript/jest update in the meantime: infinitered/jest-preset-ignite@f32890d

Like this, I got the boilerplate to work on both, iOS and Android.

@nirre7
Copy link
Contributor Author

nirre7 commented Sep 26, 2018

@marcelkalveram thanks for the help!

The jest setup now works great!

However there are still issues with the IOS + SplashPagePatch...
Did you get it to work with the PR? I can't get it work, even with the corrected "RNSplashScreen".
The splashpage won't work at all and I get Montserrat font related errors.

I have a stashed updated of the patch that adds UIAppFonts with Montserrat references and the UIStatusBarStyle styling but I can't get it to work...

Any help is appreciated
Thanks in advance

@marcelkalveram
Copy link
Contributor

@nirre7 how do you run the boilerplate locally? I ran into some errors when ignite silently failed at adding this boilerplate as a plugin. So when installing it from a local folder I had to tweak the following line inboilerplate.js from

const boilerplate = parameters.options.b || parameters.options.boilerplate || 'ignite-ir-boilerplate-bowser'

to

const boilerplate = 'ignite-ir-boilerplate-bowser'

and then I run ignite new exampleApp -b ./ignite-ir-boilerplate-bowser.

Or otherwise it would skip a bunch of install steps such as react-native link because it would search for my local repository on npm and fail.

@nirre7
Copy link
Contributor Author

nirre7 commented Sep 26, 2018

I run it as:
ignite new BowserLocal --boilerplate=<PATH_TO_LOCAL_REPO>/ignite-ir-boilerplate-bowser
For example:
ignite new BowserLocal --boilerplate=/Users/niso/dev/github/ignite-ir-boilerplate-bowser

I've ran it a couple of times with --debug or --verbose (forget which one ...) and it seems to run everything locally.

@marcelkalveram
Copy link
Contributor

marcelkalveram commented Sep 26, 2018

@nirre7 oh, ok! I do get the following error if I don't apply the little patch/hack I mentioned:
SRC_PATH/ignite-ir-boilerplate-bowser is not available on npm

If that's not the issue, I'd also have to investigate further.

@nirre7
Copy link
Contributor Author

nirre7 commented Sep 26, 2018

Oh thats weird. Everything builds and works on Android without any hacks for me.

Ignite doctor
JavaScript
node 8.11.4 /Users/niso/.nvm/versions/node/v8.11.4/bin/node
npm 5.6.0 /Users/niso/.nvm/versions/node/v8.11.4/bin/npm
yarn 1.9.4 /Users/niso/.nvm/versions/node/v8.11.4/bin/yarn

React Native
react-native-cli 2.0.1

Ignite
ignite 2.1.1 /Users/niso/.nvm/versions/node/v8.11.4/bin/ignite

Android
java 1.8.0_144 /Users/niso/.jenv/shims/java
android home - /Users/niso/Library/Android/sdk

iOS
xcode 10.0

@marcelkalveram
Copy link
Contributor

@nirre7 My assumption is that it's something xCode 10-related. For me it works fine on xCode 9.4 and I can't reproduce the issue you have.

It looks like the RN team is aware of xCode 10 problems and working on fixing them for the 0.58 release: facebook/react-native#19573

@nirre7
Copy link
Contributor Author

nirre7 commented Sep 26, 2018

Could definitely have something todo with xcode 😞 .... Given that Android works etc.

I'm going to try to downgrade to xcode 9.x and then I'll update / create a new PR when 0.58 is out 👍

@kevinvangelder
Copy link
Member

@ryanlntn was able to get one of our projects to build in Xcode 10 after upgrading the project to RN 0.57, so maybe he has some ideas.

@ryanlntn
Copy link
Member

I definitely had some linking issues after upgrading but for the most part I was able to resolve them by updating Cocoapods. I would still recommend against using react-native with XCode 10 right now. It seems incredibly unstable. I've been having pretty consistent issues with the installer failing unless I reboot the simulator.

Deleted too much in previous commit ...
…rplate-bowser

# Conflicts:
#	boilerplate/package.json.ejs
@nirre7
Copy link
Contributor Author

nirre7 commented Sep 27, 2018

Finally 🎉

Got it to work after downgrading to XCode 9.4.1.
Now both IOS and Android works.
Didn't need to use the hack mentioned earlier in the thread.

Would be great to get this reviewed and merged 😄

Copy link
Contributor

@morgandonze morgandonze left a comment

Choose a reason for hiding this comment

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

Looks good apart from the error that @jamonholmgren referenced. I'm going to add some code on your fork that will execute the workaround described here

@nirre7
Copy link
Contributor Author

nirre7 commented Oct 8, 2018

@mlaco @jamonholmgren Did it work when you ran the bundler in a separate terminal?

I might be wrong (since there been so many issues with babel and regressions, it's hard to keep track of everything) but I think that error might be related to xcode 10 rather than the metro bundler,

The regression issue I mentioned earlier is regarding the bundler and the RN hmr issue.

@morgandonze
Copy link
Contributor

@nirre7 sorry for the delay

I was able to get this up and running using a separate terminal. However, I encountered a different problem along the way not mentioned here. If it only affects me, maybe it can be ignored.

The error message:

The bundle identifier of the application could not be determined.
Ensure that the application's Info.plist contains a value for CFBundleIdentifier.
Print: Entry, ":CFBundleIdentifier", Does Not Exist

and is fixed by changing the project settings:

If Xcode > 9 run command react-native upgrade
then
1.Go to File -> Project settings
2.Click the Advanced button
3.Select "Custom" and select "Relative to Workspace" in the pull down
4.Change "Build/Products" to "build/Build/Products"

I'm still trying to figure out what to make of this for now. I'll update again.

@morgandonze
Copy link
Contributor

I updated my macOS and now I am getting the RN hmr issue. Finally I was able to reproduce it!

@nirre7
Copy link
Contributor Author

nirre7 commented Oct 11, 2018

Seems they're prepping 0.57.3 now. Hopefully it will fix "everything" 🤞. react-native-community/releases#46
I'll update the PR as soon as it hits the npm repo.

@morgandonze
Copy link
Contributor

I hear that RN 0.57.3 is coming very soon, so I'll revisit this once it drops!

@nirre7 nirre7 changed the title Update Ignite Bowser to RN 0.57.2 Update Ignite Bowser to RN 0.57.3 Oct 12, 2018
@nirre7
Copy link
Contributor Author

nirre7 commented Oct 12, 2018

Created and ran ignite app on both IOS and Android and it works. None of the earlier issues. Did not have to do any workarounds.
Tested it with xcode 9.4.1.

@morgandonze
Copy link
Contributor

@nirre7 great! I'll test it myself now. I'll get back to you asap

@morgandonze
Copy link
Contributor

Apparently I had some environment issues to sort out; sorry for the delay. I've tested it, and it looks good to go, so I'll approve!

@jamonholmgren
Copy link
Member

jamonholmgren commented Oct 12, 2018

Does this work on Xcode 10? I'm not sure I'll have time to test today. Note that React Native 0.57.3 still has the react-native init issue.

@jamonholmgren
Copy link
Member

@mlaco reports that it works! I'm good to go. 👍

@morgandonze morgandonze merged commit 1449a2c into infinitered:master Oct 12, 2018
@ugurozturk
Copy link

image

@jamonholmgren
@mlaco
@nirre7

@nirre7
Copy link
Contributor Author

nirre7 commented Oct 13, 2018

@ugurozturk How are you running ignite? Is it on an existing RN / Ignite project?

Run:
ignite doctor
And check if you get any errors.

FYI It won't work with ignite new SomeApp => choose Bowser and supply latest RN version, since the Bowser version on npm (atm) is based on 0.55 and it will not work with the latest RN since there has been changes in Info.plist between version 0.55 and latest. I.e the old patch won't work.

If you really want to use the latest bowser ask someone at ignite to publish the latest to npm OR clone the repo and run it locally:
ignite new BowserBasedApp --boilerplate=PATH_TO_CLONE/ignite-ir-boilerplate-bowser
👍

@ugurozturk
Copy link

@nirre7
image

@nirre7
Copy link
Contributor Author

nirre7 commented Oct 13, 2018

Hmm.. Weird. Only difference is node version and that you run windows + another git client.
I don't have a windows box to try on.

When git is trying to apply the patch/hunks it fails.

You could try to comment out the rnpm config in package.json in the boilerplate folder in the bowser clone. It might be that the info.plist file gets updated in the wrong order.
Could also be that the git client don't like the patch file...

@ugurozturk
Copy link

ugurozturk commented Oct 13, 2018

the path is not correct. Perhaps this is makes it fail. The folder is java/com//MainActivity.java but it looks into java/com/SplashScreenPatch/MainActivity.java

file:splash-screen-patch
image

@morgandonze
Copy link
Contributor

@ugurozturk sorry about this. I'm trying to recruit help on this, because I'm not that great with the internals of Ignite yet.

@nirre7
Copy link
Contributor Author

nirre7 commented Oct 16, 2018

Those lines has been the same since the patch file was created it seems.
Can you create an app with bowser based on 0.55 (i.e current)?
Seems to be environment related, windows + installed git client.

@morgandonze
Copy link
Contributor

So it looks like we need to modify that path, and others like it, to match the app name. I'll see if I can manage that!

@morgandonze
Copy link
Contributor

Oh, it looks like this code is meant to do exactly that. I guess the question would be why it didn't work in this case?

@morgandonze
Copy link
Contributor

morgandonze commented Oct 16, 2018

@ugurozturk where is that diff file in your screenshot from? The boilerplate itself, or inside the app you generated?

After looking into it, I see how it works now. Ignite copies the patch to your app directory (at which point it will still have the path /android/app/src/main/java/com/SplashScreenPatch/MainActivity.java. But then it changes SplashScreenPatch in that path to the name of your app.

@ugurozturk
Copy link

@nirre7 Ofc without local boilerplate and bower test :
image

@mlaco it was location on new project

  1. git clone https://github.com/infinitered/ignite-ir-boilerplate-bowser.git
  2. ignite new TestProjev2 --boilerplate=/ignite-ir-boilerplate-bowser
  3. <new_project_path>\patches\splash-screen\splash-screen.patch

Yet it seems changed now o.O
image

Still idk why i having this error

@jamonholmgren
Copy link
Member

@mlaco We may need something like this for Xcode 10:

https://github.com/infinitered/ir-boilerplate-barebones/blob/master/boilerplate.js#L35-L36

@morgandonze
Copy link
Contributor

@ugurozturk I tried to reproduce this using your steps, but it worked for me. I did however, receive the message about the patch being corrupted, after I modified the patch. I assume you didn't modify yours though?

@ugurozturk
Copy link

@mlaco Well idk why, yet i tried this boilerplate on my Debian Server. And everything looks good in the Debian Server.

image

@morgandonze
Copy link
Contributor

morgandonze commented Oct 18, 2018

@jamonholmgren I found this script in this issue about XCode build fails that we might be able to use. I'm trying it out now.

There are a few other proposed fixes in the issue too, if that doesn't work.

(btw, I'm not working solely on this issue. I just came across that script and realized it could be useful here)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants