Skip to content
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

[Wallet] Create run_app.sh script to facilitate app development #2186

Merged
merged 6 commits into from
Dec 16, 2019

Conversation

jmrossy
Copy link
Contributor

@jmrossy jmrossy commented Dec 11, 2019

Description

Create a new, simpler way to run the app for development that solves the following pain points:

  1. WalletKit out of sync with target network
  2. Firebase URL out of sync with target network
  3. Android app must be restarted on first run
  4. Simpler way to enable hot reload
  5. Simpler way to skip unnecessary steps

Still TODO:

  • convert E2E tests to use this
  • Address iOS pain points
  • Update blockchain-api to use net firebase integration url

Tested

Locally on Mac.
Testing on Linux still TODO, may do later.

Related issues

Backwards compatibility

N/A

@jmrossy jmrossy requested a review from a team December 11, 2019 15:45
Copy link
Contributor

@i1skn i1skn left a comment

Choose a reason for hiding this comment

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

Great work!! LGTM from the command API perspective (I'm no expert in sh scripts).

@@ -5,17 +5,15 @@
"license": "Apache-2.0",
"private": true,
"scripts": {
"start": "react-native start",
"start:bg": "react-native start &",
"start": "./scripts/run_app.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify: yarn start now would fail with a proposal to specify a platform, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Maybe it's not worth it to have this still here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine. I would assume, this is the first candidate to run when somebody has no experience with the codebase.

Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Great you took the time for this! 🎉

packages/mobile/package.json Show resolved Hide resolved
Comment on lines +115 to +116
# TODO have iOS build and start from command line
echo -e "\nFor now ios must be build and run from xcode\nStarting RN bundler\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do yarn react-native run-ios --simulator "iPhone 11". Last time I checked it worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it didn't work but I'll try again :)

Do you want it to start it though? I thought you preferred Xcode? Maybe there's a way to trigger Xcode actions from command line?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, iPhone 11 only works from Xcode 11,
I just checked, and yarn react-native run-ios --simulator "iPhone Xʀ" works.

I think it makes sense to build it from the command line by default and have another switch to open Xcode too. What do you think?

packages/mobile/scripts/run_app.sh Outdated Show resolved Hide resolved
packages/mobile/scripts/run_app.sh Outdated Show resolved Hide resolved
Comment on lines +100 to +104
echo "Starting packager in new terminal"
RN_START_CMD="cd `pwd`;yarn react-native start"
OSASCRIPT_CMD="tell application \"Terminal\" to do script \"$RN_START_CMD\""
echo "FULLCMD: $OSASCRIPT_CMD"
osascript -e "$OSASCRIPT_CMD"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be cool to do that only if the packager is not already running.
I usually start the packager separately so I can launch it with custom options like --reset-cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does reset cache do? Would you like a flag for the script that sets it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It removes cached files, makes sure all files are transformed again. Fixes edge cases when changing babel config or transformer config.
Not sure it's worth having a flag for now here.

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #2186 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2186      +/-   ##
==========================================
+ Coverage   74.82%   74.88%   +0.05%     
==========================================
  Files         276      279       +3     
  Lines        7771     7797      +26     
  Branches      978      690     -288     
==========================================
+ Hits         5815     5839      +24     
- Misses       1839     1842       +3     
+ Partials      117      116       -1
Flag Coverage Δ
#mobile 74.88% <ø> (+0.05%) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/paymentRequest/utils.ts 57.14% <0%> (-14.29%) ⬇️
...es/mobile/src/escrow/EscrowedPaymentListScreen.tsx 90.32% <0%> (-9.68%) ⬇️
...ages/mobile/src/escrow/EscrowedPaymentListItem.tsx 68.88% <0%> (-3.67%) ⬇️
.../paymentRequest/IncomingPaymentRequestListItem.tsx 72.34% <0%> (-1.47%) ⬇️
packages/mobile/src/home/NotificationBox.tsx 74.46% <0%> (-1.07%) ⬇️
packages/mobile/src/app/ErrorScreen.tsx 92.3% <0%> (-0.55%) ⬇️
packages/mobile/src/send/SendConfirmation.tsx 71.42% <0%> (-0.54%) ⬇️
packages/mobile/src/transactions/NoActivity.tsx 95.83% <0%> (-0.17%) ⬇️
packages/mobile/src/app/ErrorBoundary.tsx 100% <0%> (ø) ⬆️
packages/mobile/src/import/ImportContacts.tsx 74.5% <0%> (ø) ⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03fb0ac...e4150a3. Read the comment docs.

@jmrossy jmrossy merged commit fe4a672 into master Dec 16, 2019
@jmrossy jmrossy deleted the rossy/wa-better-dev-ux branch December 16, 2019 12:19
aaronmgdr added a commit that referenced this pull request Dec 16, 2019
* master: (40 commits)
  Add utils as a dep (#2252)
  [Wallet] Create run_app.sh script to facilitate app development (#2186)
  [Wallet] Fix persisted data loss on iOS (#2249)
  adds Testing TSX /react on the web (#2229)
  Update running-a-validator.md (#2259)
  Add ts-ignore (#2254)
  Revert "Revert "Upgrade TS version (#2196)" (#2251)" (#2255)
  Force same bignumber version (#2256)
  Fix missing Text on website (#2237)
  Revert "Upgrade TS version (#2196)" (#2251)
  [Wallet] Refer to Celo Lite as "Data Saver" mode (#2232)
  Fix using requester instead of requestee at Outgoing notifications (#2240)
  Use correct phone placeholder depending on the country on joining Celo view (#2061)
  Update Attestation Bot Docker Image (#2231)
  Baklava phase 1.1 .env file (#2226)
  [Docs] Fix typo (#2225)
  Upgrade Dependencies and get react hooks working (#2203)
  Update attestation service docker images (#2202)
  Updates TME docker images (#2200)
  Remove walletkit from celotool transactions commands (#2206)
  ...

# Conflicts:
#	packages/web/package.json
aaronmgdr added a commit that referenced this pull request Dec 16, 2019
* master:
  Add utils as a dep (#2252)
  [Wallet] Create run_app.sh script to facilitate app development (#2186)
  [Wallet] Fix persisted data loss on iOS (#2249)
  adds Testing TSX /react on the web (#2229)
  Update running-a-validator.md (#2259)
  Add ts-ignore (#2254)
  Revert "Revert "Upgrade TS version (#2196)" (#2251)" (#2255)
  Force same bignumber version (#2256)
  Fix missing Text on website (#2237)
  Revert "Upgrade TS version (#2196)" (#2251)
  [Wallet] Refer to Celo Lite as "Data Saver" mode (#2232)
  Fix using requester instead of requestee at Outgoing notifications (#2240)
  Use correct phone placeholder depending on the country on joining Celo view (#2061)
  Update Attestation Bot Docker Image (#2231)
  Baklava phase 1.1 .env file (#2226)
  [Docs] Fix typo (#2225)
  Upgrade Dependencies and get react hooks working (#2203)
  Update attestation service docker images (#2202)
  Updates TME docker images (#2200)
  Remove walletkit from celotool transactions commands (#2206)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure app is correctly configured before running
3 participants