-
Notifications
You must be signed in to change notification settings - Fork 12
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
Breaking: Remove Expo SDK 51 config, switch to app.config.ts #462
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
app.json
config for SDK 52app.json
config for SDK 52
app.json
config for SDK 52
bin/expo-setup.js
Outdated
const config: ExpoConfig = ${JSON.stringify(appJson.expo, null, 2) | ||
.replace(/"([^"]+)":/g, '$1:') | ||
.replace(/"(.*?)"/g, `'$1'`) | ||
.replace(/([}\]])(\s*[}\]])/g, '$1,$2') | ||
.replace(/}(\s+\])/g, '},$1') | ||
.replace(/(?<!,)(\n\s*[}\]])/g, ',$1')}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch to Prettier for this, don't write our own home-grown formatting code
Prettier is installed in bin/install.js
, so you should be able to import + use this version:
eslint-config-upleveled/bin/install.js
Lines 157 to 163 in 0e66443
// The VS Code Prettier extension uses Prettier v2 internally, | |
// but Preflight uses the latest Prettier version, which causes | |
// crashes and formatting conflicts: | |
// https://github.com/prettier/prettier-vscode/pull/3069#issuecomment-1817589047 | |
// https://github.com/prettier/prettier-vscode/issues/3298 | |
// https://github.com/upleveled/preflight/issues/429 | |
'prettier', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can run the Prettier with await promisify(exec)('npx prettier --write app.config.ts');
This command runs local Prettier and formats the app.config.ts
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use the Prettier API since we're already executing JS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use Prettier, ESLint needs to be installed first. However, the current Command Line Cheatsheet places the ESLint installation as the final step (step 4). Since we require Prettier to format app.config.ts
during the transition from app.json
, this causes a problem.
Changing the order of steps in the cheatsheet is not an option.
Potential solutions without creating a CLI:
- Stick with
app.json
and wait for Expo to address the issue and usesapp.config.ts
- Use a script with the
replace
method instead of Prettier to formatapp.config.ts
- Provide the script for students to copy, run manually, and delete after execution (maybe bad)
- Manually switching or reordering the setup steps introduces too much complexity and room for errors
The best solution right now seems to be using the replace
method, avoiding unnecessary dependencies, avoid changing the Command Line Cheatsheet and reducing the risk of errors.
Reverted and used replace
instead again.
aac3141
Update:
Also tested with a echo
command to create the file, but students still have to remove the app.json
file manually, or use a rm -rf
command
echo -e "import { type ExpoConfig } from \"expo/config\";\n\nconst config: ExpoConfig = {\n name: '$(\jq -r '.expo.name' app.json)',\n slug: '$(\jq -r '.expo.slug' app.json)',\n version: '1.0.0',\n orientation: 'portrait',\n icon: './assets/images/icon.png',\n scheme: 'myapp',\n userInterfaceStyle: 'automatic',\n newArchEnabled: true,\n ios: {\n supportsTablet: true,\n },\n android: {\n adaptiveIcon: {\n foregroundImage: './assets/images/adaptive-icon.png',\n backgroundColor: '#ffffff',\n },\n },\n web: {\n bundler: 'metro',\n output: 'static',\n favicon: './assets/images/favicon.png',\n },\n plugins: [\n 'expo-router',\n [\n 'expo-splash-screen',\n {\n image: './assets/images/splash-icon.png',\n imageWidth: 200,\n resizeMode: 'contain',\n backgroundColor: '#ffffff',\n },\n ],\n ],\n experiments: {\n typedRoutes: true,\n },\n};\n\nexport default config;" > app.config.ts
rm app.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We came to the solution to add Prettier in the expo-setup
script, and format the app.config.ts
file with it. f8cdd74
bin/expo-setup.js
Outdated
const expoConfig = | ||
`import { ExpoConfig } from "expo/config"; | ||
|
||
const config: ExpoConfig = ${JSON.stringify(appJson.expo, null, 2) | ||
.replace(/"([^"]+)":/g, '$1:') | ||
.replace(/"(.*?)"/g, `'$1'`) | ||
.replace(/([}\]])(\s*[}\]])/g, '$1,$2') | ||
.replace(/}(\s+\])/g, '},$1') | ||
.replace(/(?<!,)(\n\s*[}\]])/g, ',$1')}; | ||
|
||
export default config; | ||
`.trim() + '\n'; | ||
|
||
await writeFile('app.config.ts', expoConfig, 'utf8'); | ||
console.log('✅ Converted app.json to app.config.ts'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're taking over converting to app.config.ts
, we need to have a code comment in this code to point to an Expo issue or PR that create-expo-app
/ create-expo
(mention both names in the issue / PR if you create it) should be the config format that is generated - active maintainers of create-expo-app
can be tagged on this issue / PR (maybe Simek is active, not sure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bin/expo-setup.js
Outdated
appJson.expo.plugins = [ | ||
[ | ||
'expo-build-properties', | ||
{ | ||
ios: { | ||
newArchEnabled: true, | ||
}, | ||
android: { | ||
newArchEnabled: true, | ||
}, | ||
}, | ||
], | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ProchaLu is this equivalent with the new setting you mentioned comes by default:
This PR updates the project configuration to align with changes introduced in Expo SDK 52. With SDK 52, the extra configuration steps required for SDK 51 are no longer needed. Running pnpm create expo-app@latest . now initializes the app.json file with:
expo.newArchEnabled = true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Boolean value that indicates whether the app should use the new architecture. Defaults to true.
https://docs.expo.dev/versions/latest/config/app/#newarchenabled
There is still the possibility to add the specific iOS / Android newArchEnabled
settings, but we want to use the newArchEnabled = true
in both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that doesn't mention that it's the same as expo-build-properties
anywhere, but it's probably the same, so let's stick with that
if you can easily verify that the app is running in the New Architecture, that would be a helpful additional step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the docs from "Enable the New Architecture in an existing project":
SDK 52:
To enable it on both Android and iOS, use the newArchEnabled at the root of the expo object in your app config. You can selectively enable it for a single platform by setting, for example, "android": { "newArchEnabled": true }.
SDK 51:
To enable it, you need to install the expo-build-properties plugin and set newArchEnabled on target platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok good, that's probably enough proof, thanks!
Co-authored-by: Karl Horky <[email protected]>
Co-authored-by: Karl Horky <[email protected]>
…eveled into remove-expo-parts
bin/expo-setup.js
Outdated
import isPlainObject from 'is-plain-obj'; | ||
|
||
execSync('pnpm add --save-dev prettier', { stdio: 'inherit' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- let's stick with async APIs, instead of the
*Sync
functions - because this is unusual (using pnpm to install via
exec
, dynamic import below) let's add a comment above this why it's better (mention colocation and maybe ease of deleting the code later when it's not needed anymore, add a link to the Expo tracking issue/PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Issue https://github.com/upleveled/courses/issues/3000
Follow up PR
Breaking change: the new config introduced in this PR is SDK 52 configuration for the New Architecture, so this should only be used with Expo SDK 52+
This PR updates the project configuration to align with changes introduced in Expo SDK 52. With SDK 52, the extra configuration steps required for SDK 51 are no longer needed. Running
pnpm create expo-app@latest .
now initializes theapp.json
file with:expo.newArchEnabled = true
expo.experiments.typedRoutes = true
Additionally, we switch from
app.json
toapp.config.ts
to make the config more dynamic, add TS support, and make it easier to manage environment-specific settings.