chore: remove postinstall steps from CI to speed up prepare-install action #11219
Conversation
|
Thanks for the PR, @kirkwaiblinger! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
View your CI Pipeline Execution ↗ for commit 1fb95d6.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
💯 💯 yes!
IMO we should merge this ASAP and see what breaks. If I'm understanding the nx philosophy right then all of our scripts should know to run their dependencies first. In a perfect world I think that means we'd be able to eventually delete the postinstall script and have it all automagically inferred. 🔪
|
There were some confusing nx config changes just merged in #11135 which have caused conflicts with this one |
|
I'll share my thoughts in a loom to try and bring everyone on the same page |
For now, the postinstall script also does a bit of devx setup, in that it installs husky git hooks. So we probably still want it for that? For this PR, I went with But yeah possibly the part after the |
|
The PR and loom are here: #11226 |
4e575ae
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11219 +/- ##
=======================================
Coverage 90.91% 90.91%
=======================================
Files 501 501
Lines 50869 50869
Branches 8382 8382
=======================================
Hits 46248 46248
Misses 4606 4606
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| @@ -78,7 +78,7 @@ jobs: | |||
| uses: ./.github/actions/prepare-install | |||
| with: | |||
| node-version: ${{ env.PRIMARY_NODE_VERSION }} | |||
| - run: yarn generate-configs | |||
There was a problem hiding this comment.
directly running yarn generate-configs seems to execute the yarn script directly, ignoring the dependsOn. So, I think yarn nx run generate-configs executes the nx task instead, which includes the necessary dependsOn.
PR Checklist
Overview
Take all of the following with a grain of salt since this is not my area of expertise...
The postinstall script can take a while (locally, at least) since it builds the whole project from scratch. I'm not sure to what extent this is mitigated by nx caching in CI, but, locally, the difference between
SKIP_POSTINSTALL=true yarnandyarnis 9 seconds vs 68 seconds... So I'd think that explains the 1 minute prepare-install time mentioned in #11205For CI steps that need the tasks done by
yarn build, nx's dependency graph should still execute the required build steps, meaning that we'll mostly just be shifting running time out of the prepare-install step, and into the main CI job. So, likely not a huge overall speedup for those cases. However, for actions that don't have any upstream build dependencies, like semantic-breaking-changes-pr, I would expect this to be a significant speedup.I'd be surprised if this didn't break anything if merged to main, tbh. Previously, many of the nx build dependencies weren't accurately declared, and, therefore, several commands depended on the "build everything" task built into
yarn install. Note that @aryaemami59 has made quite a few fixes in this area, though! So perhaps things will work.Merging this would also introduce a downside that CI failures would be harder for community contributors to debug since they would generally not know to do
SKIP_POSTINSTALL=true yarn && yarn my-failing-commandto replicate the behavior in CI.