-
Notifications
You must be signed in to change notification settings - Fork 13
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
build: speedup sample app installing local SDK #177
Conversation
Pull request title looks good 👍! If this pull request gets merged, it will not cause a new release of the software. Example: If this project's latest release version is All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time. This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...This project uses a special format for pull requests titles. Don't worry, it's easy! This pull request title should be in this format:
If your pull request introduces breaking changes to the code, use this format:
where
Examples:
Need more examples? Want to learn more about this format? Check out the official docs. Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests. |
Apps/APN/package.json
Outdated
}, | ||
"dependencies": { | ||
"@react-native-async-storage/async-storage": "^1.17.11", | ||
"@react-native-clipboard/clipboard": "^1.11.2", | ||
"@react-native-community/push-notification-ios": "^1.10.1", | ||
"@react-navigation/native": "^6.0.10", | ||
"@react-navigation/native-stack": "^6.6.2", | ||
"customerio-reactnative": "../..", | ||
"customerio-reactnative": "file:../../customerio-reactnative-1690212805.tgz", |
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.
From script below, 1690212805
looks like the timestamp which changes every time. So I don't think we should be changing dependency here, it may fail on customer's machines and may be in CI builds as well?
Also, we'll then probably have to revert this every time before pushing? Considering this saves a lot of our testing time while development, I'm happy with it. But if we can find a way that can prevent this being pushed to git, that will be awesome.
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.
That's a good idea. I didn't consider the idea that if we commit the file like it is now, it would not allow others to compile the sample app.
One idea to resolve this is to use npm
instead of yarn
for the sample apps (we can continue to use yarn for the SDK) because npm
might not have the same cache issue that yarn
has which is why we have more complex configuration here. We could then change the package.json
file to use a static file path: "customerio-reactnative": "file:../../customerio-reactnative.tgz"
instead of it being dynamic like it is now.
I know yarn
is very popular in RN so I am not sure if npm
will work as well. But, it is worth a try.
Note: The use of using .tgz
over ../../
is (1) it speeds up yarn install
for the sample apps and (2) it's installing the SDK the same way that customers installing our module from npmjs.com does. If we use ../../
, we run the risk of passing QA but missing files when we deploy to npm. If we use the .tgz
file to install from, we are QA testing against a set of files that will be pushed to production.
I'll try to get this working.
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.
it may fail on customer's machines and may be in CI builds as well?
I thought this is for sample apps only, can this fail on customer builds as well?
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.
This PR is using "customerio-reactnative": "file:../../customerio-reactnative-1690212805.tgz"
as the path to the SDK dependency to install.
The file customerio-reactnative-1690212805.tgz
only exists on my computer because my computer generated it. If you were to checkout this branch on your machine and try running yarn install
, installing would fail for you.
So, I think Rehan is talking about how customers who try to build the sample app (not SDK) will have problems.
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.
Yeah, I was talking about customers trying to build the sample app and not the SDK, sorry for the confusion.
I like the idea of having static file path, if it works well, I think we can use npm
instead of yarn
for sample apps 👍🏻
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 have moved over to npm for the sample apps and it has resolved this problem. Our sample apps can now use a hard-coded value in package.json
"postinstall": "pod update --project-directory=ios", | ||
"preinstall": "npm run dev:update" |
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 added these to make installing easier for devs.
If someone was to clone the repo and try to run npm install
for the first time for any of our sample apps, they would see errors because (1) ../../customerio-reactnative.tgz
does not yet exist and (2) pod install
might have been forgetten to execute.
By adding these scripts, by running npm install
, we automatically solve both of those problems for you.
"expo": "~42.0.1", | ||
"expo-splash-screen": "~0.11.2", | ||
"expo-status-bar": "~1.0.4", | ||
"expo-updates": "~0.8.1", | ||
"react": "18.2.0", | ||
"react-dom": "16.13.1", | ||
"react-dom": "^18.2.0", |
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.
Updated this because npm was giving me a lot of warnings about dependencies not matching up.
echo "Running pre-deploy script to compile code" | ||
yarn pre-deploy | ||
echo "pre-deploy step complete" |
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.
Made this change so that the dev:update
script and this deployment script run the same commands for compiling the RN SDK. This helps improve our QA testing by installing the local SDK into sample apps the same way that customers install our SDK in production.
Ready for re-review. Also, I improved the speed from 10 seconds to ~3 seconds. |
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.
Look okay, didn't run it so don't know for sure. One call-out that I have would be to see if it doesn't break the deployment of the code (missing dependency etc) because I see that the script is being affected too and that the script is run from multiple sources, git actions, manually, etc.
When you make an update to the SDK source code and you want to test that change in one of the sample apps, you have to run
yarn update customerio-reactnative
to force yarn to re-install the local copy of the SDK. This step takes many minutes to run and slows down development.I used
--verbose
the last time I ranyarn update...
and noticed that mostly why it was taking so long was that yarn was installing and caching the sample apps and all of it's subdirectories. This is what was taking up too much time during install.So, this refactor uses
npm pack
to build the SDK with only the files that are needed to install the SDK. We then tell npm to install the CIO SDK from the built .tgz file instead of../../
.From testing, you can now update the local RN SDK source code in a sample app in ~3 seconds. It used to be between 5-10 minutes for me.