Skip to content
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

Update ts configs, add diff-based linter. #995

Merged
merged 3 commits into from
Apr 24, 2024
Merged

Conversation

goodov
Copy link
Member

@goodov goodov commented Apr 9, 2024

Sec-review to add tsx https://github.com/brave/reviews/issues/1575

Changes in this PR:

  1. Add Prettier config for all files in the repository so it can be used natively via VSCode extension and as a part of npm run lint.
  2. Update lint packages to most recent versions.
  3. Add lint-staged integration to support diff linting with arbitrary executables. This will allow us to perform full study validation as a part of a single npm run lint call. lint-staged can also be integrated into pre-commit or pre-push git hooks, but after some experiments I decided to postpone this integration.
  4. Update tsconfig and eslint configs to run type checks on src/.

Related issue brave/brave-browser#33654

@goodov goodov force-pushed the update-projects-add-linters branch 15 times, most recently from f99a5ed to 4a31dff Compare April 9, 2024 12:31
@goodov goodov marked this pull request as ready for review April 9, 2024 12:40
@goodov goodov requested a review from atuchin-m April 9, 2024 12:40
"protobufjs-cli": "^1.1.2",
"style-loader": "^3.3.3",
"ts-jest": "^29.1.1",
"ts-loader": "^9.4.4",
"tsx": "4.7.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

tsx is superior over ts-node. I spend a lot of time fighting ts-node when I tried to run esm+commonjs packages in the same app. tsx just works.

@@ -2,32 +2,28 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

module.exports = {
settings: {
react: {
version: 'detect',
},
},
extends: ['standard-with-typescript', 'prettier', 'plugin:react/recommended'],
Copy link
Member Author

Choose a reason for hiding this comment

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

prettier stays here to disable eslint rules that prettier covers.

"../node_modules/@types",
],
"typeRoots": ["../node_modules/@types"],
"jsx": "react-jsx",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It src/web specific thing, isn't it? We shouldn't use React outside web.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is added so tsc --noEmit -p src can compile all sources in src/ without issues.

This config parameter is a no-op for other projects that don't use jsx/tsx files.

@goodov goodov requested a review from petemill April 9, 2024 14:49
@goodov goodov force-pushed the update-projects-add-linters branch 4 times, most recently from 7571421 to 69c9b78 Compare April 10, 2024 13:51
@goodov goodov force-pushed the update-projects-add-linters branch from 69c9b78 to 2bd93a5 Compare April 10, 2024 13:59
atuchin-m
atuchin-m previously approved these changes Apr 22, 2024
package.json Outdated
"serve:dev": "webpack serve -c src/web/webpack.config.js --hot --mode=development",
"test": "jest --config=src/jest.config.js",
"tracker": "node src/finch_tracker/build/finch_tracker/main.js",
"typecheck": "tsc --noEmit"
Copy link
Member

Choose a reason for hiding this comment

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

Will this be run as a blocker for merging? Otherwise there is no type-checking for the node code.

petemill
petemill previously approved these changes Apr 23, 2024
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

I think this is a good solution, as long as npm run typecheck is used as a workflow to prevent merging code that fails to compile.

@goodov goodov dismissed stale reviews from petemill and atuchin-m via 87e155b April 24, 2024 06:37
Copy link
Contributor

[puLL-Merge] - brave/brave-variations@995

Description

This PR includes various build system and code style improvements. It adds new npm scripts for linting, cleaning, and type checking. It also integrates Prettier for consistent code formatting across the project. The ESLint configuration has been updated and a new lint-staged setup ensures only relevant files are linted on commit.

Changes

Changes

  • .github/: Formatting changes to workflow YAML files
  • .gitignore: Adds **/build/ pattern to exclude build output directories
  • .prettierignore: New file to exclude certain files from Prettier formatting
  • .prettierrc.js: New Prettier configuration file at the root level
  • README.md: Minor formatting changes
  • package.json:
    • Adds new devDependencies for linting/formatting
    • Adds new scripts for cleaning, linting, type checking
  • src/.eslintrc.js: Updates ESLint config and ignores
  • src/.gitignore: Adds *.failed pattern
  • src/: Updates imports to use shortest path and be sorted
  • src/scripts/: New directory with scripts for cleaning and linting
  • src/tsconfig-lint.json: Removed in favor of root tsconfig
  • src/web/: CSS and HTML formatting changes
  • tsconfig.json: Moved to root level and updated

Security Hotspots

No major security hotspots identified in this change. The updates are mostly related to the development workflow and code styling. A few minor notes:

  1. The new clean and lint scripts execute shell commands, but the inputs are controlled and low risk.

  2. The Webpack and Jest config files execute code, but are part of the existing build system. No new risks introduced.

  3. Overall, the stricter linting, type checking and consistent code style enabled by this PR can help reduce the chances of bugs and vulnerabilities being introduced.

@goodov goodov requested a review from atuchin-m April 24, 2024 07:07
@goodov goodov merged commit 1415b9d into main Apr 24, 2024
6 checks passed
@goodov goodov deleted the update-projects-add-linters branch April 24, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants