Update createApp to no longer use individual commands #33

stevehanson · 2024-02-23T23:47:51Z

This PR addresses tickets: Architecture of "create" vs individual commands and How to prevent flakiness when versions change?.

I recommend reading the context from those tickets for background information. The issues this PR seeks to address are:

Create architecture

Issues

The create command currently runs a collection of isolated individual commands. These commands sometimes modify similar files, like package.json, App.tsx, test files, etc. Because these commands touch the same files, it becomes difficult to "layer" these changes on top of each other. Each command is unable to make assumptions about the state of the file and must instead have a way to mutate the existing file. A secondary issue is that it becomes difficult to reason about what the final output of the create command is without actually creating a new app (no easy way to visualize how the individual commands layer together).

Solution

The create command has been refactored to no longer run any other commands. Instead, we have a boilerplate directory that we copy directly to the new app directory. After that, we perform a few string substitutions to insert the app name in the appropriate places (package.json, app.json) and then install dependencies with the specified package manager. That's it!

The templates/boilerplate directory is the entire app! You can literally cd templates/boilerplate then run npm install and npm run ios or npm run test:all to run the app straight from there while updating it! This is a huge improvement in the dev experience and greatly simplifies understanding of what we're generating. This PR does not introduce any changes in the app that gets generated -- it's identical to what it was before. I generated the boilerplate directory in this PR by creating an app using our old method and then copying the result to boilerplate.

Preventing flakiness

Issues

Previously, Belt has been using the latest version of Expo, and the latest version of any other dependencies it installs. Always being on the latest and greatest is certainly desired, but it comes at the cost of stability. If any dependency updates to a new major or minor version, there is a chance that Belt might quit working. This actually happened when Expo 50 was released (see #31).

Solution

We need to balance being up to date with being stable. While we want both, stability should be a non-negotiable, so let's prioritize stability and then find ways to simplify the dependency upgrade process so we can stay as up-to-date as possible. If we can streamline updating Belt to support newer versions of Expo or other dependencies to take just a few minutes, then we are more likely to be able to keep up-to-date.

codeofdiego

Just added some comments and suggestions.

Regarding the overall strategy I do like the idea of having a boilerplate as it makes it safer for us to generate applications with less chance of errors out of the box. However, I'm still keen to a hybrid where we have the option to pass a --latest flag and generate the app from create-expo, this would be particularly helpful if Belt falls behind on Expo version (which I don't expect to happen 😄). Regardless I believe we can maintain it with the boilerplate and update when necessary.

One thing I had in mind is, how can we tackle the difference in Expo versions when we are generating vs running commands on the repo? For instance, if I generate the project from the boilerplate on Expo 50, but then some time after I run the add-notifications command with dependencies that expect a newer 51 version? Should we lock dependency versions on independent commands based on the currently supported version of Expo in Belt?

bin/diff-expo.js

codeofdiego · 2024-02-29T17:47:39Z

bin/diff-expo.js

+  await exec(`git clean -fxd`);
+  await exec(`cp -r ../expo-${newVersion}/* .`);
+  execSync(`git diff`, { stdio: 'inherit' });
+}


What do you think of removing the generated project files at the end of the script? This would make sure the script leaves the project folder clean. Additionally, we could also add a --keep-files flag to not delete them if we want to inspect them closer.

I'm inclined to keep iterating on this script in a separate PR. I don't think it's perfect as is, and figuring out how we will streamline Expo upgrades merits more thought.

Update, I found that this script is not actually necessary. I removed this script and added documentation in contributing.md for how we can upgrade Expo in the project with: yarn add expo@latest or yarn upgrade-interactive --latest followed by npx expo install --fix to ensure all installed package versions are compatible with the installed Expo version.

codeofdiego · 2024-02-29T18:36:58Z

src/util/copyTemplateDirectory.ts

  srcDir: string,
  destinationDir: string,
+  substitutions: Record<string, string>,
  variables: object,
 ) {
  const filenames = await getFiles(srcDir);
  // eslint-disable-next-line no-restricted-syntax
  for await (const filename of filenames) {


What do you think of doing await Promise.all(filenames.map(async (filename) => { in order to stick to array iterators rule and remove that eslint-disable line?

Good call! Updated in e6043cd.

codeofdiego · 2024-02-29T18:42:15Z

templates/boilerplate/.eslintrc.js

+  extends: [
+    '@thoughtbot/eslint-config/native',
+    '@thoughtbot/eslint-config/typescript',
+  ],


Suggested change

],

],

plugins: ['import'],

rules: {

'import/order': [

'error',

{

'newlines-between': 'always',

groups: [

'builtin',

'external',

'internal',

'parent',

'sibling',

'index',

],

alphabetize: {

order: 'asc',

caseInsensitive: true,

},

},

],

},

The notifications command does introduce some rules for the import/order that weren't present in our @thoughtbot/eslint-config that will be helpful to when we add new imports to files dynamically and need to rearrange them to keep things organized. What do you think of making it part of the boilerplate?

Since we didn't have these rules before, how would you feel about capturing this in a ticket so we can implement it in a separate PR?

codeofdiego · 2024-02-29T18:43:19Z

templates/boilerplate/.gitignore

+# typescript
+*.tsbuildinfo
+
+/.cache


Should we include the android and ios folders in the .gitignore file by default or let that be added by specific commands when needed?

Good call! Updated in af1970a.

Previously, the flow was: 1. Copy the template directory to the destination directory 2. Iterate through each of the files copied 1. Perform string substitutions 2. Render ETA templates (and remove .eta) When copying the "boilerplate" directory, we ideally ignore any files that are "gitignored" by this repo. This can be especially helpful with the "boilerplate" directory in the case where a developer has run the app and has a node_modules directory there. The new approach is: 1. Iterate through each of the files in the template directory 2. Filter out files that are gitignored (based on gitignore contents passed in) 3. For each file, read contents, perform substitutions, copy to destination

Remove diff-expo as npx expo install --fix already handles this

stevehanson · 2024-03-08T14:55:25Z

@codeofdiego thanks so much for your review! I think I've addressed your feedback, so this is ready for re-review when you get a chance.

stevehanson · 2024-03-08T19:27:49Z

One thing I had in mind is, how can we tackle the difference in Expo versions when we are generating vs running commands on the repo? For instance, if I generate the project from the boilerplate on Expo 50, but then some time after I run the add-notifications command with dependencies that expect a newer 51 version? Should we lock dependency versions on independent commands based on the currently supported version of Expo in Belt?

Good questions! I don't know the answer yet, but at a high level, we should probably be using npx expo install for dependencies or installing dependencies at our preferred version and then following up with npx expo install --check or npx expo install --fix to ensure the dependency is compatible. I see your push notifications PR is already using npx expo install, so it seems like it's in a pretty good spot! This would be a good topic to have a conversation on separately from this PR, since this issue exists with or without this change.

stevehanson · 2024-03-08T19:28:05Z

This has been open for a couple weeks, so I'm going to go ahead and merge it.

codeofdiego reviewed Feb 29, 2024

View reviewed changes

Update createApp to no longer use individual commands

df2b087

stevehanson force-pushed the create-rearch branch from 76c81fd to 45899c0 Compare March 1, 2024 20:46

stevehanson force-pushed the create-rearch branch from 45899c0 to 1ae864a Compare March 1, 2024 21:08

stevehanson marked this pull request as ready for review March 1, 2024 21:14

Update check scripts logging

76bc418

stevehanson force-pushed the create-rearch branch from 1ae864a to 76bc418 Compare March 1, 2024 21:17

Stephen Hanson added 3 commits March 1, 2024 15:21

Include ios and android in boilerplate gitignore

af1970a

Add instructions for updating dependencies

b60185e

Remove diff-expo as npx expo install --fix already handles this

Use promise.all instead of await with for...of

e6043cd

stevehanson force-pushed the create-rearch branch from ec28f16 to e6043cd Compare March 2, 2024 22:23

codeofdiego mentioned this pull request Mar 5, 2024

Add Push Notifications command #34

Merged

stevehanson requested a review from codeofdiego March 8, 2024 14:54

stevehanson force-pushed the create-rearch branch 2 times, most recently from 7dd3571 to edcbf28 Compare March 8, 2024 17:28

Update message

7fcfab2

stevehanson force-pushed the create-rearch branch from edcbf28 to 7fcfab2 Compare March 8, 2024 18:06

stevehanson merged commit 760fb63 into main Mar 8, 2024
2 checks passed

stevehanson deleted the create-rearch branch March 8, 2024 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update createApp to no longer use individual commands #33

Update createApp to no longer use individual commands #33

stevehanson commented Feb 23, 2024 •

edited

Loading

codeofdiego left a comment

codeofdiego Feb 29, 2024

stevehanson Mar 1, 2024 •

edited

Loading

stevehanson Mar 8, 2024

codeofdiego Feb 29, 2024

stevehanson Mar 8, 2024

codeofdiego Feb 29, 2024

stevehanson Mar 1, 2024

codeofdiego Feb 29, 2024

stevehanson Mar 1, 2024

stevehanson commented Mar 8, 2024

stevehanson commented Mar 8, 2024

stevehanson commented Mar 8, 2024

-  ],
+  ],
+  plugins: ['import'],
+  rules: {
+    'import/order': [
+      'error',
+      {
+        'newlines-between': 'always',
+        groups: [
+          'builtin',
+          'external',
+          'internal',
+          'parent',
+          'sibling',
+          'index',
+        ],
+        alphabetize: {
+          order: 'asc',
+          caseInsensitive: true,
+        },
+      },
+    ],
+  },

Update createApp to no longer use individual commands #33

Update createApp to no longer use individual commands #33

Conversation

stevehanson commented Feb 23, 2024 • edited Loading

Create architecture

Issues

Solution

Preventing flakiness

Issues

Solution

codeofdiego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevehanson Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevehanson commented Mar 8, 2024

stevehanson commented Mar 8, 2024

stevehanson commented Mar 8, 2024

stevehanson commented Feb 23, 2024 •

edited

Loading

stevehanson Mar 1, 2024 •

edited

Loading