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

chore: upgrade expo and restructure app #82

Merged
merged 2 commits into from
May 5, 2021
Merged

chore: upgrade expo and restructure app #82

merged 2 commits into from
May 5, 2021

Conversation

salmanm
Copy link
Contributor

@salmanm salmanm commented May 5, 2021

Fixes #81

@@ -14,41 +14,42 @@
"dependencies": {
"@expo-google-fonts/didact-gothic": "^0.1.0",
"@expo-google-fonts/poppins": "^0.1.0",
"@react-native-async-storage/async-storage": "1.15.1",
"@react-native-async-storage/async-storage": "^1.13.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

expo cli did this downgrade

"expo-secure-store": "~10.1.0",
"expo-status-bar": "~1.0.4",
"expo-web-browser": "~9.1.0",
"firebase": "8.2.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had 8.4.1 (before I made it 7.9.0) and expo upgrade util automatically made it 8.2.3 so I kept it to avoid firebase warning.

"react-native-web": "~0.13.12",
"url-otpauth": "^2.0.0",
"uuid": "8.3.2"
},
"devDependencies": {
"@babel/core": "~7.13.15",
"@babel/core": "~7.9.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, expo upgrade util downgraded it. We could up it back, but may be it downgraded for some reason!?

@salmanm salmanm requested a review from simoneb May 5, 2021 07:48
Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

The package versions are fine, in general it's good to stick to whatever expo wants. firebase was an exception really because at some point the version that expo wanted didn't work.

I'm not sure about the whole app restructuring, unless it comes from expo itself. If not, unless you have a good reason to change the app structure I would consider keeping it the same for the sake of remaining consistent with the expo template, possibly making further upgrades easier because there may be some assumptions that expo makes about the file system layout or contents of certain files.

"backgroundColor": "#FFFFFF"
}
},
"web": {
"favicon": "./assets/favicon.png"
"favicon": "./src/assets/favicon.png"
Copy link
Member

Choose a reason for hiding this comment

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

are the changes in this file stuff that the expo upgrade did?

@@ -1,5 +1,5 @@
{
"main": "node_modules/expo/AppEntry.js",
"main": "./src/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

is this also the expo upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I wanted to keep all the app related files under /src and other stuff outside. So had to do this.

@salmanm
Copy link
Contributor Author

salmanm commented May 5, 2021

some assumptions that expo makes about the file system layout or contents of certain files

Yes, and when deviating from that, their docs say to use registerRootComponent, which I've done.

unless you have a good reason to change the app structure

Well, I'm new in RN so no strong opinions here. But to me it makes sense to keep the bootstrapping files separate from the app code. So when upgrading expo, if the root directory has more files in it, they don't clutter the app code structutre.

I mean, we had .vscode, .expo, etc directories at the same level as context, which is very app specific directory. IMHO, it's confusing.

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.

update expo SDK
2 participants