Format all file types supported by Prettier #1686
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 41 files out of 123 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThis pull request encompasses a comprehensive set of formatting and configuration updates across multiple files in the React on Rails project. The changes primarily focus on code style consistency, updating dependency versions, and removing or modifying configuration files. The modifications span various aspects of the project, including CircleCI configuration, ESLint rules, GitHub Actions workflows, Prettier settings, and TypeScript type definitions. While most changes are cosmetic, some involve updating dependency versions and removing deprecated configurations. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexeyr-ci
force-pushed
the
alexeyr/format-typescript
branch
2 times, most recently
from
af9f19e to
1ffb14e
Compare
alexeyr-ci
force-pushed
the
alexeyr/format-typescript
branch
2 times, most recently
from
e54f3c2 to
22a4598
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
lib/generators/react_on_rails/templates/.eslintrc (1)
Line range hint
2-24: Add TypeScript-specific ESLint configuration.Given that one of the PR objectives is to enhance TypeScript formatting, consider adding TypeScript-specific ESLint configuration:
extends: - eslint-config-shakacode - prettier + - prettier/@typescript-eslint + - plugin:@typescript-eslint/recommended plugins: - react + - @typescript-eslint globals: __DEBUG_SERVER_ERRORS__: true __SERVER_ERRORS__: true env: browser: true node: true mocha: true rules: no-console: 0 # https://github.com/benmosher/eslint-plugin-import/issues/340 import/no-extraneous-dependencies: 0 # because template cannot find react-on-rails import/no-unresolved: 0
♻️ Duplicate comments (3)
.github/workflows/main.yml (3)
76-76:⚠️ Potential issueFix deprecated GitHub Actions commands.
These lines use the deprecated
set-outputcommand. Apply the same fix as suggested in examples.yml.Also applies to: 136-136
🧰 Tools
🪛 actionlint (1.7.4)
76-76: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
70-70: 🛠️ Refactor suggestionImprove shell script reliability.
These lines use the potentially problematic
A && B || Cpattern. Apply the same fix as suggested in examples.yml.Also applies to: 153-153, 155-155
🧰 Tools
🪛 actionlint (1.7.4)
70-70: shellcheck reported issue in this script: SC2015:info:1:60: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
157-167:⚠️ Potential issueFix numeric comparison in shell script.
The comparison operator should be
-ltinstead of<. Apply the same fix as suggested in examples.yml.🧰 Tools
🪛 actionlint (1.7.4)
157-157: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
🧹 Nitpick comments (5)
node_package/src/reactHydrateOrRender.ts (1)
8-24: Track TODO for React 18 upgrade.The code includes a TODO comment indicating this complex optional dependency handling can be simplified once React is upgraded to version 18 or higher.
Would you like me to create an issue to track this TODO for future cleanup when React is upgraded?
node_package/src/ReactOnRails.ts (2)
69-72: Consider using template literals for better readability.The error message could be more readable using template literals instead of string concatenation.
- throw new Error( - 'Called ReactOnRails.registerStoreGenerators with a null or undefined, rather than ' + - 'an Object with keys being the store names and the values are the store generators.', - ); + throw new Error(` + Called ReactOnRails.registerStoreGenerators with a null or undefined, rather than + an Object with keys being the store names and the values are the store generators. + `);
259-261: Consider using template literals for consistency.For consistency with other error messages in the file, consider using template literals.
- throw new Error( - 'streamServerRenderedReactComponent is only supported when using a bundle built for Node.js environments', - ); + throw new Error(` + streamServerRenderedReactComponent is only supported when using a bundle built for Node.js environments + `);lib/generators/react_on_rails/templates/.eslintrc (1)
Line range hint
18-24: Review semi rule configuration with Prettier.The
semi: 0rule might conflict with Prettier's semicolon formatting. Consider removing this rule and letting Prettier handle semicolon formatting consistently.rules: no-console: 0 # https://github.com/benmosher/eslint-plugin-import/issues/340 import/no-extraneous-dependencies: 0 # because template cannot find react-on-rails import/no-unresolved: 0 - semi: 0package-scripts.yml (1)
72-76: Consider separating JSON and YAML formatting.While combining JSON and YAML formatting works, consider separating them for:
- Better control over file patterns
- Independent verification
- Clearer error reporting
json: default: - description: Run prettier on JSON and YAML files. - script: rm -rf packages/vm-renderer/tests/tmp && prettier "**/*.@(json|yaml)" "**/.*rc" --write + description: Run prettier on JSON files. + script: prettier "**/*.json" "**/.*rc" --write listDifferent: - description: Check if any JSON or YAML files would change by running prettier. - script: prettier "**/*.@(json|yaml)" "**/.*rc" --check + description: Check if any JSON files would change by running prettier. + script: prettier "**/*.json" "**/.*rc" --check +yaml: + default: + description: Run prettier on YAML files. + script: prettier "**/*.yaml" --write + listDifferent: + description: Check if any YAML files would change by running prettier. + script: prettier "**/*.yaml" --check
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
.circleci/config.yml(0 hunks).eslintrc(0 hunks).github/workflows/examples.yml(2 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(2 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/rspec-package-specs.yml(1 hunks).prettierignore(1 hunks).prettierrc(1 hunks).scss-lint.yml(1 hunks).travis.yml(2 hunks)lib/generators/react_on_rails/templates/.eslintrc(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)node_package/src/Authenticity.ts(1 hunks)node_package/src/ComponentRegistry.ts(1 hunks)node_package/src/ReactOnRails.ts(6 hunks)node_package/src/StoreRegistry.ts(4 hunks)node_package/src/buildConsoleReplay.ts(2 hunks)node_package/src/clientStartup.ts(10 hunks)node_package/src/context.ts(1 hunks)node_package/src/createReactOutput.ts(3 hunks)node_package/src/handleError.ts(2 hunks)node_package/src/isRenderFunction.ts(1 hunks)node_package/src/isServerRenderResult.ts(1 hunks)node_package/src/reactApis.ts(1 hunks)node_package/src/reactHydrateOrRender.ts(1 hunks)node_package/src/serverRenderReactComponent.ts(8 hunks)node_package/src/serverRenderUtils.ts(2 hunks)node_package/src/streamServerRenderedReactComponent.ts(4 hunks)node_package/src/types/index.ts(6 hunks)package-scripts.yml(1 hunks)package.json(1 hunks)rakelib/examples_config.yml(1 hunks)spec/dummy/.prettierignore(0 hunks)spec/dummy/.prettierrc(0 hunks)spec/dummy/client/.eslintrc(0 hunks)spec/dummy/client/.jscsrc(1 hunks)spec/dummy/config/locales/en.yml(1 hunks)spec/dummy/config/shakapacker.yml(1 hunks)spec/dummy/config/storage.yml(0 hunks)spec/react_on_rails/fixtures/i18n/locales/de.yml(1 hunks)spec/react_on_rails/fixtures/i18n/locales/en.yml(1 hunks)
💤 Files with no reviewable changes (6)
- spec/dummy/.prettierrc
- spec/dummy/client/.eslintrc
- spec/dummy/config/storage.yml
- spec/dummy/.prettierignore
- .eslintrc
- .circleci/config.yml
✅ Files skipped from review due to trivial changes (23)
- node_package/src/reactApis.ts
- .github/workflows/package-js-tests.yml
- spec/react_on_rails/fixtures/i18n/locales/en.yml
- rakelib/examples_config.yml
- spec/dummy/client/.jscsrc
- .github/workflows/lint-js-and-ruby.yml
- node_package/src/handleError.ts
- node_package/src/createReactOutput.ts
- node_package/src/context.ts
- node_package/src/serverRenderReactComponent.ts
- spec/dummy/config/shakapacker.yml
- node_package/src/isRenderFunction.ts
- spec/dummy/config/locales/en.yml
- node_package/src/streamServerRenderedReactComponent.ts
- .travis.yml
- .github/workflows/rspec-package-specs.yml
- node_package/src/serverRenderUtils.ts
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
- node_package/src/ComponentRegistry.ts
- node_package/src/Authenticity.ts
- .scss-lint.yml
- spec/react_on_rails/fixtures/i18n/locales/de.yml
- node_package/src/clientStartup.ts
🧰 Additional context used
🪛 GitHub Actions: Lint JS and Ruby
node_package/src/buildConsoleReplay.ts
[warning] Code style issues found. File needs to be formatted using Prettier.
🪛 actionlint (1.7.4)
.github/workflows/examples.yml
64-64: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
72-72: shellcheck reported issue in this script: SC2015:info:1:43: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
74-74: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
.github/workflows/main.yml
70-70: shellcheck reported issue in this script: SC2015:info:1:60: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
72-72: shellcheck reported issue in this script: SC2209:warning:1:18: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
76-76: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
136-136: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
153-153: shellcheck reported issue in this script: SC2015:info:1:43: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
155-155: shellcheck reported issue in this script: SC2015:info:1:60: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
157-157: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
171-171: shellcheck reported issue in this script: SC2209:warning:1:18: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
🪛 Biome (1.9.4)
node_package/src/reactHydrateOrRender.ts
[error] 38-38: Do not depend on the value returned by the function ReactDOM.render().
The returned value is legacy and future versions of React might return that value asynchronously.
Check the React documentation for more information.
(lint/correctness/noRenderReturnValue)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (24)
node_package/src/reactHydrateOrRender.ts (3)
1-6: LGTM! Clean imports and type definitions.The imports are well-organized with proper type imports, and the
HydrateOrRenderTypetype definition is clear and precise.
26-28: LGTM! Clean hydration implementation.The code elegantly handles both modern (
hydrateRoot) and legacy (hydrate) React implementations.
41-47: LGTM! Clean and readable implementation.The function signature is well-formatted across multiple lines, improving readability while maintaining a simple and correct implementation.
node_package/src/ReactOnRails.ts (4)
108-108: LGTM! Improved TypeScript type definition syntax.The change from comma to semicolon in the object type definition follows TypeScript's preferred syntax.
124-124: LGTM! Proper use of template literals for error message.The error message correctly uses template literals with JSON.stringify for formatting the invalid options.
230-234: LGTM! Improved readability of render method.The multi-line formatting enhances readability while maintaining necessary type assertions.
312-312: LGTM! Good practice to export types.Exporting types makes them available to consumers and follows TypeScript module best practices.
lib/generators/react_on_rails/templates/.eslintrc (1)
2-4: Verify ESLint and Prettier configuration compatibility.The configuration extends both
eslint-config-shakacodeandprettier. Let's verify there are no rule conflicts between these configurations.✅ Verification successful
ESLint and Prettier configuration is correctly set up ✅
The configuration extends
eslint-config-shakacodeandprettierin the correct order, with Prettier last to ensure its rules take precedence. The custom rule overrides don't conflict with Prettier's formatting rules.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential ESLint and Prettier conflicts # Check if eslint-config-shakacode has its own Prettier configuration npm info eslint-config-shakacode # Look for any existing eslint-prettier configuration files fd -t f -e json -e js -e yaml -e yml . | rg -i 'prettier|eslint'Length of output: 970
Script:
#!/bin/bash # Check ESLint and Prettier configurations # Show content of the template .eslintrc cat lib/generators/react_on_rails/templates/.eslintrc # Find and show content of any Prettier config files fd -t f -e json -e js -e yaml -e yml . | rg -i 'prettier' --files-with-matches | xargs cat # Find any ESLint rule configurations that might conflict with Prettier rg -i "rules.*:|\"rules\":|'rules':" -A 10Length of output: 4905
node_package/src/StoreRegistry.ts (4)
19-22: LGTM! Improved error message formatting.The use of template literals and proper indentation enhances readability.
Line range hint
44-48: LGTM! Clear and informative error message.The multi-line format with numbered points makes the error message easy to understand and act upon.
54-57: LGTM! Consistent error message formatting.Maintains consistency with other error messages in the file.
75-78: LGTM! Consistent error message formatting.Maintains consistency with other error messages in the file.
node_package/src/isServerRenderResult.ts (1)
3-9: LGTM! Type guard implementations are correct.The type guard functions are well-implemented:
isServerRenderHashcorrectly checks for the presence of required propertiesisPromiseproperly verifies the existence of thethenmethodAlso applies to: 12-15
node_package/src/buildConsoleReplay.ts (2)
7-8: LGTM! Interface property formatting is correct.The
Consoleinterface properties are properly formatted according to TypeScript conventions.🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] Code style issues found. File needs to be formatted using Prettier.
13-16: LGTM! Function signatures are properly formatted.Both
consoleReplayandbuildConsoleReplayfunctions have their parameters correctly formatted on separate lines, improving readability.Also applies to: 51-58
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] Code style issues found. File needs to be formatted using Prettier.
node_package/src/types/index.ts (3)
32-35: LGTM! Type definitions are properly formatted.The
AuthenticityHeadersandStoreGeneratortypes are correctly formatted according to TypeScript conventions.Also applies to: 37-37
53-65: Documentation improvements enhance code clarity.The added JSDoc comments provide clear explanations and examples for render functions.
Line range hint
83-94: LGTM! Export statement is properly formatted.The type exports are correctly organized and formatted.
.prettierrc (2)
8-8: Updated to use the new Prettier option name.The
jsxBracketSameLineoption has been correctly renamed tobracketSameLineas per Prettier's latest conventions.
11-22: LGTM! Configuration overrides are properly structured.The overrides section is well-organized with:
- Proper formatting for CSS/SCSS files
- JSON file configuration
- New YAML parser for rc files
.prettierignore (2)
16-26: Well-documented ignore entries.The new ignore entries are clearly documented with explanatory comments, making maintenance easier:
- ERB syntax issues in YAML files
- Intentionally invalid test fixtures
- Non-YAML files
- ESLint compatibility issues
13-13: Consider the implications of broadening the generated files pattern.The pattern change from
**/generated/**to**/*generated*will now ignore any file containing "generated" in its path, not just files within a "generated" directory. This could potentially ignore files unintentionally.✅ Verification successful
Pattern change has no current impact but should be documented
The broader pattern
**/*generated*currently doesn't affect any existing files in the codebase. However, it would be helpful to document the intention behind this change to help future maintainers understand which files should be ignored.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find files that would be newly ignored by the broader pattern fd -t f | grep -P "generated" | grep -vP "/generated/"Length of output: 52
Script:
#!/bin/bash # First verify repository content and both patterns echo "=== Repository files ===" fd -t f | head -n 5 echo -e "\n=== Files matching old pattern (**/generated/**) ===" fd -t f | grep -P "/generated/" echo -e "\n=== Files matching new pattern (**/*generated*) ===" fd -t f | grep -i "generated"Length of output: 469
package-scripts.yml (2)
36-43: Well-structured concurrent formatting setup.The format script has been improved to:
- Run formatters concurrently for better performance
- Use clear naming for each concurrent task
- Separate concerns by file type
54-60: Good addition of TypeScript formatting support.The TypeScript formatting configuration:
- Covers both .ts and .tsx files
- Includes both write and check modes
- Follows the same pattern as other formatters
| export function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType { | ||
| if (supportsRootApi) { | ||
| const root = reactDomClient.createRoot(domNode); | ||
| root.render(reactElement); | ||
| return root; | ||
| } | ||
|
|
||
| // eslint-disable-next-line react/no-render-return-value | ||
| return ReactDOM.render(reactElement, domNode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the legacy render return value usage.
While the code includes an ESLint disable comment, the usage of ReactDOM.render's return value is discouraged as it's a legacy pattern. Future versions of React might return this value asynchronously.
Consider refactoring to avoid depending on the return value:
export function reactRender(domNode: Element, reactElement: ReactElement): RenderReturnType {
if (supportsRootApi) {
const root = reactDomClient.createRoot(domNode);
root.render(reactElement);
return root;
}
- // eslint-disable-next-line react/no-render-return-value
- return ReactDOM.render(reactElement, domNode);
+ ReactDOM.render(reactElement, domNode);
+ // Return null or undefined to indicate legacy mode
+ return null;
}This change would require updating the RenderReturnType to allow null values:
type RenderReturnType = Root | null;🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Do not depend on the value returned by the function ReactDOM.render().
The returned value is legacy and future versions of React might return that value asynchronously.
Check the React documentation for more information.
(lint/correctness/noRenderReturnValue)
| run: | | ||
| echo -e "Already installed $(google-chrome --version)\n" | ||
| MINIMUM_REQUIRED_CHROME_VERSION=75 | ||
| INSTALLED_CHROME_MAJOR_VERSION="$(google-chrome --version | tr ' .' '\t' | cut -f3)" | ||
| if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then | ||
| wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | ||
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | ||
| sudo apt-get update | ||
| sudo apt-get install google-chrome-stable | ||
| echo -e "\nInstalled $(google-chrome --version)" | ||
| fi |
There was a problem hiding this comment.
Fix numeric comparison in shell script.
Use -lt instead of < for numeric comparisons in shell scripts.
- if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then
+ if [[ $INSTALLED_CHROME_MAJOR_VERSION -lt $MINIMUM_REQUIRED_CHROME_VERSION ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| echo -e "Already installed $(google-chrome --version)\n" | |
| MINIMUM_REQUIRED_CHROME_VERSION=75 | |
| INSTALLED_CHROME_MAJOR_VERSION="$(google-chrome --version | tr ' .' '\t' | cut -f3)" | |
| if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then | |
| wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | |
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | |
| sudo apt-get update | |
| sudo apt-get install google-chrome-stable | |
| echo -e "\nInstalled $(google-chrome --version)" | |
| fi | |
| run: | | |
| echo -e "Already installed $(google-chrome --version)\n" | |
| MINIMUM_REQUIRED_CHROME_VERSION=75 | |
| INSTALLED_CHROME_MAJOR_VERSION="$(google-chrome --version | tr ' .' '\t' | cut -f3)" | |
| if [[ $INSTALLED_CHROME_MAJOR_VERSION -lt $MINIMUM_REQUIRED_CHROME_VERSION ]]; then | |
| wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | |
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | |
| sudo apt-get update | |
| sudo apt-get install google-chrome-stable | |
| echo -e "\nInstalled $(google-chrome --version)" | |
| fi |
🧰 Tools
🪛 actionlint (1.7.4)
74-74: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
| - name: yalc publish for react-on-rails | ||
| run: yalc publish | ||
| - name: Install Ruby Gems for package | ||
| run: bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve shell script reliability.
The pattern A && B || C can lead to unexpected behavior. Use an explicit if-then-else construct instead.
- run: bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ run: |
+ bundle lock --add-platform 'x86_64-linux'
+ if bundle check --path=vendor/bundle; then
+ echo "Bundle check passed"
+ else
+ bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 | |
| run: | | |
| bundle lock --add-platform 'x86_64-linux' | |
| if bundle check --path=vendor/bundle; then | |
| echo "Bundle check passed" | |
| else | |
| bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 | |
| fi |
🧰 Tools
🪛 actionlint (1.7.4)
72-72: shellcheck reported issue in this script: SC2015:info:1:43: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
package.json
Outdated
| "eslint-config-prettier": "^10.0.1", | ||
| "eslint-config-shakacode": "^16.0.1", | ||
| "eslint-plugin-import": "^2.29.1", | ||
| "eslint-plugin-jsx-a11y": "^6.8.0", | ||
| "eslint-plugin-prettier": "^3.4.1", | ||
| "eslint-plugin-prettier": "^5.2.3", | ||
| "eslint-plugin-react": "^7.33.2", | ||
| "jest": "^29.7.0", | ||
| "jest-environment-jsdom": "^29.7.0", | ||
| "jsdom": "^22.1.0", | ||
| "nps": "^5.9.3", | ||
| "prettier": "^2.8.8", | ||
| "prettier-eslint-cli": "^5.0.0", | ||
| "prettier": "^3.4.2", |
There was a problem hiding this comment.
💡 Codebase verification
Incompatible dependency versions detected
The current eslint version (^7.32.0) is incompatible with [email protected], which requires eslint >= 8.0.0. You need to:
- Update eslint to version 8 or higher along with these changes
- Or downgrade eslint-plugin-prettier to a version compatible with eslint v7
🔗 Analysis chain
Verify compatibility with major version updates.
The PR includes significant version bumps:
- prettier v2 → v3
- eslint-config-prettier v7 → v10
- eslint-plugin-prettier v3 → v5
These updates may introduce breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes and compatibility
echo "Checking prettier changelog for breaking changes..."
curl -s https://raw.githubusercontent.com/prettier/prettier/main/CHANGELOG.md | grep -A 10 "3.0.0"
echo "Verifying peer dependency requirements..."
npm info [email protected] peerDependencies
npm info [email protected] peerDependencies
Length of output: 2812
Script:
#!/bin/bash
# Check eslint version in package.json
echo "Checking eslint version..."
cat package.json | jq '.dependencies.eslint, .devDependencies.eslint'
Length of output: 149
alexeyr-ci
force-pushed
the
alexeyr/format-typescript
branch
2 times, most recently
from
729e94c to
67d4506
Compare
| "eslint-config-shakacode": "^16.0.1", | ||
| "eslint-plugin-import": "^2.29.1", | ||
| "eslint-plugin-jsx-a11y": "^6.8.0", | ||
| "eslint-plugin-prettier": "^3.4.1", | ||
| "eslint-plugin-prettier": "^4.2.1", |
There was a problem hiding this comment.
There is also v5, but it requires ESLint >=8.0.0, and should be updated together with that.
alexeyr-ci
force-pushed
the
alexeyr/format-typescript
branch
from
67d4506 to
bc11297
Compare
| @@ -1,7 +1,6 @@ | |||
| extends: | |||
| - eslint-config-shakacode | |||
| - prettier | |||
| - prettier/react | |||
There was a problem hiding this comment.
Removed in eslint-prettier-config.
| @@ -16,79 +16,79 @@ jobs: | |||
| versions: ['oldest', 'newest'] | |||
| runs-on: ubuntu-22.04 | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
There was a problem hiding this comment.
You may prefer to hide whitespace while reviewing.
| @@ -31,7 +31,7 @@ before_install: | |||
| - sudo apt-get install -y xvfb libappindicator1 fonts-liberation | |||
| - wget https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb | |||
| - sudo dpkg -i google-chrome*.deb | |||
| - "/sbin/start-stop-daemon --start --quiet --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -ac -screen scn 1600x1200x16" | |||
| - '/sbin/start-stop-daemon --start --quiet --pidfile /tmp/custom_xvfb_99.pid --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -ac -screen scn 1600x1200x16' | |||
There was a problem hiding this comment.
Should this file be deleted instead?
package.json
Outdated
| "eslint-plugin-react": "^7.33.2", | ||
| "jest": "^29.7.0", | ||
| "jest-environment-jsdom": "^29.7.0", | ||
| "jsdom": "^22.1.0", | ||
| "nps": "^5.9.3", | ||
| "prettier": "^2.8.8", | ||
| "prettier-eslint-cli": "^5.0.0", |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/examples.yml (3)
64-64:⚠️ Potential issueUpdate deprecated GitHub Actions command.
The
set-outputcommand is deprecated. Use the newGITHUB_OUTPUTenvironment file approach instead.- run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT + run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT🧰 Tools
🪛 actionlint (1.7.4)
64-64: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
72-72:⚠️ Potential issueImprove shell script reliability.
The pattern
A && B || Ccan lead to unexpected behavior. Use an explicit if-then-else construct instead.- run: bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 + run: | + bundle lock --add-platform 'x86_64-linux' + if bundle check --path=vendor/bundle; then + echo "Bundle check passed" + else + bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 + fi🧰 Tools
🪛 actionlint (1.7.4)
72-72: shellcheck reported issue in this script: SC2015:info:1:43: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
74-84:⚠️ Potential issueFix numeric comparison in shell script.
Use
-ltinstead of<for numeric comparisons in shell scripts.- if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then + if [[ $INSTALLED_CHROME_MAJOR_VERSION -lt $MINIMUM_REQUIRED_CHROME_VERSION ]]; then🧰 Tools
🪛 actionlint (1.7.4)
74-74: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
🧹 Nitpick comments (3)
.github/workflows/main.yml (1)
174-177: Consider using environment variables for Git configuration.Instead of hardcoding Git user information, consider using environment variables or GitHub Actions context variables.
- git config user.email "[email protected]" - git config user.name "Your Name" + git config user.email "${GITHUB_ACTOR}@users.noreply.github.com" + git config user.name "${GITHUB_ACTOR}"package-scripts.yml (2)
36-43: Fix color configuration for concurrent tasks.The
-c "yellow,magenta"configuration only specifies two colors but there are three concurrent tasks (ts, js, json). This mismatch could affect task output visibility.- -c "yellow,magenta" \ + -c "yellow,magenta,cyan" \
72-73: Improve cross-platform compatibility of cleanup command.The cleanup command
rm -rfmight fail on Windows. Consider using a cross-platform solution.- script: rm -rf packages/vm-renderer/tests/tmp && prettier "**/*.@(json|yaml)" "**/.*rc" --write + script: rimraf packages/vm-renderer/tests/tmp && prettier "**/*.@(json|yaml)" "**/.*rc" --writeMake sure to add
rimrafas a dev dependency if not already present:// package.json { "devDependencies": { + "rimraf": "^5.0.0" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.circleci/config.yml(0 hunks).eslintrc(0 hunks).github/workflows/examples.yml(2 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(2 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/rspec-package-specs.yml(1 hunks).prettierignore(1 hunks).prettierrc(1 hunks).scss-lint.yml(1 hunks).travis.yml(2 hunks)lib/generators/react_on_rails/templates/.eslintrc(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)node_package/src/buildConsoleReplay.ts(2 hunks)package-scripts.yml(1 hunks)package.json(1 hunks)rakelib/examples_config.yml(1 hunks)spec/dummy/.prettierignore(0 hunks)spec/dummy/.prettierrc(0 hunks)spec/dummy/client/.eslintrc(0 hunks)spec/dummy/client/.jscsrc(1 hunks)spec/dummy/config/locales/en.yml(1 hunks)spec/dummy/config/shakapacker.yml(1 hunks)spec/dummy/config/storage.yml(0 hunks)spec/react_on_rails/fixtures/i18n/locales/de.yml(1 hunks)spec/react_on_rails/fixtures/i18n/locales/en.yml(1 hunks)
💤 Files with no reviewable changes (6)
- spec/dummy/config/storage.yml
- spec/dummy/.prettierignore
- .eslintrc
- .circleci/config.yml
- spec/dummy/.prettierrc
- spec/dummy/client/.eslintrc
🚧 Files skipped from review as they are similar to previous changes (17)
- spec/dummy/client/.jscsrc
- spec/react_on_rails/fixtures/i18n/locales/de.yml
- lib/generators/react_on_rails/templates/.eslintrc
- .travis.yml
- spec/dummy/config/locales/en.yml
- .github/workflows/rspec-package-specs.yml
- spec/react_on_rails/fixtures/i18n/locales/en.yml
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
- .github/workflows/package-js-tests.yml
- spec/dummy/config/shakapacker.yml
- .scss-lint.yml
- .github/workflows/lint-js-and-ruby.yml
- rakelib/examples_config.yml
- .prettierrc
- package.json
- .prettierignore
- node_package/src/buildConsoleReplay.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/examples.yml
64-64: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
72-72: shellcheck reported issue in this script: SC2015:info:1:43: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
74-74: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
.github/workflows/main.yml
70-70: shellcheck reported issue in this script: SC2015:info:1:60: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
72-72: shellcheck reported issue in this script: SC2209:warning:1:18: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
76-76: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
136-136: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
153-153: shellcheck reported issue in this script: SC2015:info:1:43: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
155-155: shellcheck reported issue in this script: SC2015:info:1:60: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
157-157: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
171-171: shellcheck reported issue in this script: SC2209:warning:1:18: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (2)
package-scripts.yml (2)
54-60: Well-structured TypeScript formatting configuration!The TypeScript formatting configuration is well-implemented with appropriate glob patterns and formatting options.
63-69: Verify coverage of JavaScript files after glob pattern simplification.The script has been simplified from specific directory patterns to a general glob pattern. Let's verify that all JavaScript files are still covered by the new pattern.
✅ Verification successful
Simplified glob pattern provides better coverage for consistent code formatting
The new pattern
**/*.@(js|jsx)includes additional JavaScript/JSX files that should benefit from consistent formatting:
- Configuration files (jest.config.js, babel.config.js)
- Generator templates
- Test files
- Webpack configuration
- Fixture files
This broader coverage helps maintain consistent code style across all JavaScript files in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare old and new glob pattern coverage echo "Files that would be matched by old pattern:" find . -path "*/packages/**/*.js" -o -path "*/packages/**/*.jsx" -o -path "*/spec/dummy/client/app/**/*.js" -o -path "*/spec/dummy/client/app/**/*.jsx" -o -name "webpack.config.babel.js" -o -path "*/webpack/**/*.js" | sort echo -e "\nFiles that would be matched by new pattern:" find . -name "*.js" -o -name "*.jsx" | sort echo -e "\nComparing coverage..." diff <(find . -path "*/packages/**/*.js" -o -path "*/packages/**/*.jsx" -o -path "*/spec/dummy/client/app/**/*.js" -o -path "*/spec/dummy/client/app/**/*.jsx" -o -name "webpack.config.babel.js" -o -path "*/webpack/**/*.js" | sort) <(find . -name "*.js" -o -name "*.jsx" | sort)Length of output: 69903
Script:
#!/bin/bash # Description: Compare old and new glob pattern coverage (excluding node_modules) echo "Files matched by old pattern (excluding node_modules):" find . -path "*/packages/**/*.js" -o -path "*/packages/**/*.jsx" -o -path "*/spec/dummy/client/app/**/*.js" -o -path "*/spec/dummy/client/app/**/*.jsx" -o -name "webpack.config.babel.js" -o -path "*/webpack/**/*.js" | grep -v "node_modules" | sort echo -e "\nFiles matched by new pattern (excluding node_modules):" find . -name "*.js" -o -name "*.jsx" | grep -v "node_modules" | sort echo -e "\nFiles that would be newly included by the simplified pattern:" diff <(find . -path "*/packages/**/*.js" -o -path "*/packages/**/*.jsx" -o -path "*/spec/dummy/client/app/**/*.js" -o -path "*/spec/dummy/client/app/**/*.jsx" -o -name "webpack.config.babel.js" -o -path "*/webpack/**/*.js" | grep -v "node_modules" | sort) <(find . -name "*.js" -o -name "*.jsx" | grep -v "node_modules" | sort) | grep "^>" | sed 's/^> //'Length of output: 11690
| path: spec/dummy/vendor/bundle | ||
| key: dummy-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||
| - name: Install Ruby Gems for dummy app | ||
| run: cd spec/dummy && bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 |
There was a problem hiding this comment.
Improve shell script reliability in multiple locations.
The pattern A && B || C can lead to unexpected behavior in multiple locations. Use explicit if-then-else constructs instead.
# Line 70
- run: cd spec/dummy && bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ run: |
+ cd spec/dummy
+ bundle lock --add-platform 'x86_64-linux'
+ if bundle check --path=vendor/bundle; then
+ echo "Bundle check passed"
+ else
+ bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ fi
# Line 153
- run: bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ run: |
+ bundle lock --add-platform 'x86_64-linux'
+ if bundle check --path=vendor/bundle; then
+ echo "Bundle check passed"
+ else
+ bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ fi
# Line 155
- run: cd spec/dummy && bundle lock --add-platform 'x86_64-linux' && bundle check --path=vendor/bundle || bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ run: |
+ cd spec/dummy
+ bundle lock --add-platform 'x86_64-linux'
+ if bundle check --path=vendor/bundle; then
+ echo "Bundle check passed"
+ else
+ bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3
+ fiAlso applies to: 153-153, 155-155
🧰 Tools
🪛 actionlint (1.7.4)
70-70: shellcheck reported issue in this script: SC2015:info:1:60: Note that A && B || C is not if-then-else. C may run when A is true
(shellcheck)
| - name: Build test bundles for dummy app | ||
| run: cd spec/dummy && rm -rf public/webpack/test && yarn build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer | ||
| - id: get-sha | ||
| run: echo "::set-output name=sha::$(git rev-parse HEAD)" |
There was a problem hiding this comment.
Update deprecated GitHub Actions commands.
The set-output command is deprecated in multiple locations. Use the new GITHUB_OUTPUT environment file approach instead.
# Line 76
- run: echo "::set-output name=sha::$(git rev-parse HEAD)"
+ run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT
# Line 136
- run: echo "::set-output name=sha::$(git rev-parse HEAD)"
+ run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUTAlso applies to: 136-136
🧰 Tools
🪛 actionlint (1.7.4)
76-76: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
| run: | | ||
| echo -e "Already installed $(google-chrome --version)\n" | ||
| MINIMUM_REQUIRED_CHROME_VERSION=75 | ||
| INSTALLED_CHROME_MAJOR_VERSION="$(google-chrome --version | tr ' .' '\t' | cut -f3)" | ||
| if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then | ||
| wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | ||
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | ||
| sudo apt-get update | ||
| sudo apt-get install google-chrome-stable | ||
| echo -e "\nInstalled $(google-chrome --version)" | ||
| fi |
There was a problem hiding this comment.
Fix numeric comparison in shell script.
Use -lt instead of < for numeric comparisons in shell scripts.
- if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then
+ if [[ $INSTALLED_CHROME_MAJOR_VERSION -lt $MINIMUM_REQUIRED_CHROME_VERSION ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| echo -e "Already installed $(google-chrome --version)\n" | |
| MINIMUM_REQUIRED_CHROME_VERSION=75 | |
| INSTALLED_CHROME_MAJOR_VERSION="$(google-chrome --version | tr ' .' '\t' | cut -f3)" | |
| if [[ $INSTALLED_CHROME_MAJOR_VERSION < $MINIMUM_REQUIRED_CHROME_VERSION ]]; then | |
| wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | |
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | |
| sudo apt-get update | |
| sudo apt-get install google-chrome-stable | |
| echo -e "\nInstalled $(google-chrome --version)" | |
| fi | |
| run: | | |
| echo -e "Already installed $(google-chrome --version)\n" | |
| MINIMUM_REQUIRED_CHROME_VERSION=75 | |
| INSTALLED_CHROME_MAJOR_VERSION="$(google-chrome --version | tr ' .' '\t' | cut -f3)" | |
| if [[ $INSTALLED_CHROME_MAJOR_VERSION -lt $MINIMUM_REQUIRED_CHROME_VERSION ]]; then | |
| wget -q -O - https://dl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | |
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | |
| sudo apt-get update | |
| sudo apt-get install google-chrome-stable | |
| echo -e "\nInstalled $(google-chrome --version)" | |
| fi |
🧰 Tools
🪛 actionlint (1.7.4)
157-157: shellcheck reported issue in this script: SC2071:error:4:39: < is for string comparisons. Use -lt instead
(shellcheck)
alexeyr-ci
force-pushed
the
alexeyr/format-typescript
branch
2 times, most recently
from
3046259 to
39a04a3
Compare
alexeyr-ci
changed the title
alexeyr-ci
force-pushed
the
alexeyr/format-typescript
branch
from
39a04a3 to
b8937f1
Compare
Summary
TypeScript and YAML were not included in
nps format.*scripts.This PR:
--checkinstead of--list-different.prettier.Pull Request checklist
Add/update test to cover these changesUpdate documentationUpdate CHANGELOG fileOther information
Do not merge until #1644, #1656, and #1663 to avoid conflicts.
This change is
Summary by CodeRabbit
Release Notes
Configuration Updates
Code Style Improvements
Dependency Management
Removed Configurations
.prettierignoreand.prettierrcfrom the spec/dummy directory.Localization
Development Workflow
Note: These changes primarily focus on code style, configuration management, and dependency updates without introducing significant functional changes.