fix: Optimizations for Turborepo setup#6662
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
anthonyshew
force-pushed
the
turbo-optimizations
branch
from
f5ef2aa to
b47833b
Compare
anthonyshew
changed the title
| # We want to enforce that the actual `turbo@latest` package is used instead of a possible hijack from the user | ||
| # the `${{ needs.base.outputs.turbo_args }}` is a string substitution happening from the base job | ||
| run: npx --package=turbo@latest -- turbo prettier ${{ needs.base.outputs.turbo_args }} | ||
| run: npx --package=turbo@latest -- turbo lint:js lint:md lint:css prettier ${{ needs.base.outputs.turbo_args } |
There was a problem hiding this comment.
These tasks can all be made to run in parallel, saving some time.
| "{app,pages}/**/*.{mdx,md}", | ||
| "*.{md,mdx,json,ts,tsx,mjs,yml}" | ||
| ], | ||
| "outputs": ["coverage/**", "junit.xml"] |
There was a problem hiding this comment.
This appears to be a duplicate of the test:unit task so we can remove it and run test:unit instead.
| "lint": { | ||
| "cache": false, | ||
| "outputs": [".eslintjscache", ".eslintmdcache", ".stylelintcache"] | ||
| }, |
There was a problem hiding this comment.
We don't end up needing this task because we can use the other linting tasks in parallel instead. An example of this can be found in the package.json changes.
anthonyshew
force-pushed
the
turbo-optimizations
branch
from
60bde06 to
07d5e0f
Compare
anthonyshew
force-pushed
the
turbo-optimizations
branch
from
07d5e0f to
7a4fa1b
Compare
| ".stylelintcache", | ||
| ".prettiercache" | ||
| ] | ||
| "cache": false |
There was a problem hiding this comment.
Any time cache is false, nothing will be sent to outputs. So, we can remove these lines for clarity.
| # We want to enforce that the actual `turbo@latest` package is used instead of a possible hijack from the user | ||
| # the `${{ needs.base.outputs.turbo_args }}` is a string substitution happening from the base job | ||
| run: npx --package=turbo@latest -- turbo prettier ${{ needs.base.outputs.turbo_args }} | ||
| run: npx --package=turbo@latest -- turbo lint:js lint:md lint:css prettier ${{ needs.base.outputs.turbo_args } |
There was a problem hiding this comment.
isn't it dangerous to use the latest version? what if breaking changes are made? we should use the version our website use
There was a problem hiding this comment.
I also wondered about this but wasn't going to take the liberty to question it, especially given that the comment right above describes the motivation. I figured a core team member consciously made this choice, carefully considering the tradeoffs - and that I'm not thinking about it deeply enough to understand the hijacking surface being described. I'd be interested in learning more!
There was a problem hiding this comment.
Right, I believe we could use commit pinning here; Bug the idea behind is, I might be wrong, that NPM checks what is the latest version on the registry, and then compares what the latest locally version is installed, so it complicates even if a tiny bit more, that the attacker must match both versions if they want to poison a GitHub Action cache.
github-actions
Bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
Unit Test Coverage Report
Unit Test Report
|
|
@anthonyshew apparently the new updated command causes a loop? |
|
@ovflowd Hm, that's weird. I changed the GitHub Actions run to not use For what it's worth, I also see steps listed in the Action failure that have been removed as a part of this pull request. |
Hmmm, yes? Because the actual workflows used on a PR are the ones from main branch for security reasons. If you believe this PR is safe to merge we can give it a go and see if things pass on main |
|
@ovflowd Makes sense! I was thinking that was set up but didn't want to assume. As a brief explanation, the new Action will be using I'm confident that this is safe to merge in that regard and happy to play the part of on-call to quickly fix-forward anything that may come up. |
|
Let's see if |
|
FYI @nodejs/nodejs-website I forced a merge, since the checks would have failed regardless, since the Workflow was being updated. |
6 participants
Description
I dropped in to look at how the Node.js website team is using Turborepo and saw a few places I could help out with small optimizations.
Validation
I've dropped a few comments in the file changes. To test things locally, you can try running the commands I've altered to see the new behaviors. Additionally, you can change files in the repo to see how the caching works across runs/tasks.
One thing I can't confidently test is the GitHub Action script that I've edited. You'll notice that I collapsed two steps into one to run more
turbotasks in parallel. I'll lean on a maintainer's help there to make sure I've not created an issues/missed any context.Check List
npx turbo formatto ensure the code follows the style guide.npx turbo testto check if all tests are passing.npx turbo buildto check if the website builds without errors.