Skip to content

fix: Optimizations for Turborepo setup #6662

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.

Sign up for GitHub

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

Merged
ovflowd merged 2 commits into from
Apr 24, 2024
Merged

fix: Optimizations for Turborepo setup #6662

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a4fa1b
fix: Optimizations for Turborepo setup
anthonyshew Apr 17, 2024
fcaca4a
Missed a curly.
anthonyshew Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ You can check the items by adding an `x` between the brackets, like this: `[x]`
-->

- [ ] I have read the [Contributing Guidelines](https://github.com/nodejs/nodejs.org/blob/main/CONTRIBUTING.md) and made commit messages that follow the guideline.
- [ ] I have run `npx turbo format` to ensure the code follows the style guide.
- [ ] I have run `npx turbo test` to check if all tests are passing.
- [ ] I have run `npm run format` to ensure the code follows the style guide.
- [ ] I have run `npm run test` to check if all tests are passing.
- [ ] I have run `npx turbo build` to check if the website builds without errors.
- [ ] I've covered new added functionality with unit tests if necessary.
13 changes: 3 additions & 10 deletions .github/workflows/lint-and-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ jobs:
(github.event_name == 'pull_request_target' &&
github.event.label.name == 'github_actions:pull-request')

name: Lint
name: Quality checks
runs-on: ubuntu-latest
needs: [base]

Expand Down Expand Up @@ -116,8 +116,7 @@ jobs:
# We also use `npm i` instead of `npm ci` so that the node_modules/.cache folder doesn't get deleted
run: npm i --no-audit --no-fund --ignore-scripts --userconfig=/dev/null

- name: Run `turbo lint`
id: eslint-step
- name: Run quality checks with `turbo`
# We run the ESLint and Prettier commands on all Workflow triggers of the `Lint` job, besides if
# the Pull Request comes from a Crowdin Branch, as we don't want to run ESLint and Prettier on Crowdin PRs
# Note: Linting and Prettifying of files on Crowdin PRs is handled by the `translations-pr.yml` Workflow
Expand All @@ -127,13 +126,7 @@ jobs:
github.event.pull_request.head.ref != 'chore/crowdin')
# 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 lint ${{ needs.base.outputs.turbo_args }}

- name: Run `turbo prettier`
if: steps.eslint-step.outcome == 'success'
# 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 }
Copy link
Copy Markdown
Contributor Author

@anthonyshew anthonyshew Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tasks can all be made to run in parallel, saving some time.

Copy link
Copy Markdown
Member

@rluvaton rluvaton Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it dangerous to use the latest version? what if breaking changes are made? we should use the version our website use

Copy link
Copy Markdown
Contributor Author

@anthonyshew anthonyshew Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member

@rluvaton rluvaton Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that comment 😅

Copy link
Copy Markdown
Member

@ovflowd ovflowd Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread
ovflowd marked this conversation as resolved.
Outdated

- name: Run `tsc build`
# We want to ensure that the whole codebase is passing and successfully compiles with TypeScript
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
"lint:js": "eslint \"**/*.{js,mjs,ts,tsx}\" --cache --cache-strategy=content --cache-location=.eslintjscache",
"lint:md": "eslint \"**/*.md?(x)\" --cache --cache-strategy=content --cache-location=.eslintmdcache",
"lint:css": "stylelint \"**/*.css\" --allow-empty-input --cache --cache-strategy=content --cache-location=.stylelintcache",
"lint": "npm run lint:js && npm run lint:md && npm run lint:css",
"lint:fix": "npm run lint:js -- --fix && npm run lint:md -- --fix && npm run lint:css -- --fix",
"lint": "turbo run lint:md lint:js lint:css",
"lint:fix": "turbo run lint:md lint:js lint:css --no-cache -- --fix",
"prettier": "prettier \"**/*.{js,mjs,ts,tsx,md,mdx,json,yml,css}\" --check --cache --cache-strategy=content --cache-location=.prettiercache",
"prettier:fix": "npm run prettier -- --write",
"format": "npm run lint:fix && npm run prettier:fix",
Expand All @@ -33,7 +33,7 @@
"storybook:build": "cross-env NODE_NO_WARNINGS=1 storybook build --quiet --webpack-stats-json",
"test:unit": "cross-env NODE_NO_WARNINGS=1 jest",
"test:unit:watch": "npm run test:unit -- --watch",
"test": "npm run test:unit",
Comment thread
anthonyshew marked this conversation as resolved.
"test": "turbo test:unit",
"prepare": "husky"
},
"dependencies": {
Expand Down
40 changes: 11 additions & 29 deletions turbo.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,41 +83,33 @@
]
},
"lint:js": {
"cache": false,
"inputs": [
"{app,components,hooks,i18n,layouts,middlewares,pages,providers,types,util}/**/*.{ts,tsx,mjs}",
"{next-data,scripts,i18n}/**/*.{mjs,json}",
"{.storybook,public}/**/*.{ts,js,json}",
"*.{json,ts,tsx}"
],
"outputs": [".eslintjscache"]
},
"lint:md": {
"cache": false,
"inputs": ["{app,pages}/**/*.{md,mdx}", "*.{md,mdx}"],
"outputs": [".eslintmdcache"]
},
"lint:css": {
"cache": false,
"inputs": ["{app,components,layouts,pages,styles}/**/*.css"],
"outputs": [".stylelintcache"]
},
"lint": {
"cache": false,
"outputs": [".eslintjscache", ".eslintmdcache", ".stylelintcache"]
},
Copy link
Copy Markdown
Contributor Author

@anthonyshew anthonyshew Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"lint:fix": {
"cache": false,
"outputs": [".eslintjscache", ".eslintmdcache", ".stylelintcache"]
"cache": false
},
"prettier": {
"cache": false,
"outputs": [".prettiercache"]
},
"prettier:fix": {
"cache": false,
"outputs": [".prettiercache"]
"cache": false
},
"format": {
"cache": false,
"outputs": [
".eslintjscache",
".eslintmdcache",
".stylelintcache",
".prettiercache"
]
"cache": false
Copy link
Copy Markdown
Contributor Author

@anthonyshew anthonyshew Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any time cache is false, nothing will be sent to outputs. So, we can remove these lines for clarity.

},
"storybook": {
"cache": false,
Expand All @@ -143,16 +135,6 @@
"*.{md,mdx,json,ts,tsx,mjs,yml}"
],
"outputs": ["coverage/**", "junit.xml"]
},
"test": {
"inputs": [
"{app,components,hooks,i18n,layouts,middlewares,pages,providers,types,util}/**/*.{ts,tsx,mjs}",
"{app,components,layouts,pages,styles}/**/*.css",
"{next-data,scripts,i18n}/**/*.{mjs,json}",
"{app,pages}/**/*.{mdx,md}",
"*.{md,mdx,json,ts,tsx,mjs,yml}"
],
"outputs": ["coverage/**", "junit.xml"]
Copy link
Copy Markdown
Contributor Author

@anthonyshew anthonyshew Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be a duplicate of the test:unit task so we can remove it and run test:unit instead.

}
}
}
⚡