chore: Eject CRA #38890
chore: Eject CRA #38890
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the client-side build configuration, moving away from CRACO (Create React App Configuration Override) to a more direct Webpack and Node.js-based configuration approach. The changes involve updating build scripts, introducing new configuration files for environment handling, module resolution, and build processes, and modernizing the project's development toolchain. Changes
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/client/package.json (1)
Dependencies are compatible, but package.json needs cleanup
- The webpack (both v5.64.4 and v5.76.0) and babel (v7.16.0) versions are compatible
- However, webpack is listed twice with different versions - this should be resolved to avoid potential conflicts
🔗 Analysis chain
Line range hint
57-83: Verify webpack-related dependency versions.The added webpack and babel dependencies are essential for the CRA ejection. However, some of these dependencies have known compatibility issues in certain version combinations.
Run this script to check for version compatibility issues:
Also applies to: 93-93, 118-125
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known compatibility issues between webpack and babel dependencies npm info [email protected] peerDependencies npm info @babel/[email protected] peerDependencies npm info [email protected] peerDependenciesLength of output: 180
Script:
#!/bin/bash # Check webpack version in package.json rg '"webpack":' app/client/package.jsonLength of output: 92
🧹 Nitpick comments (24)
app/client/config/jest/fileTransform.js (3)
1-8: Remove redundant 'use strict' directive.Since this is a module, strict mode is automatically enabled.
-'use strict'; const path = require('path'); const camelcase = require('camelcase');🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
10-39: Add parameter validation and error handling.The process function should validate its parameters and handle potential errors.
process(src, filename) { + if (typeof filename !== 'string') { + throw new TypeError('Filename must be a string'); + } + const assetFilename = JSON.stringify(path.basename(filename));
24-34: Consider adding prop types validation for the SVG component.The generated React component could benefit from runtime prop validation.
ReactComponent: React.forwardRef(function ${componentName}(props, ref) { + if (process.env.NODE_ENV !== 'production') { + if (props.ref) { + console.warn('You are passing ref prop to an SVG component. Consider using the ref parameter instead.'); + } + } return {app/client/scripts/test.js (2)
1-6: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary as modules are automatically in strict mode.
-'use strict'; // Do this as the first thing so that any code reading it knows the right env.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
22-38: Consider caching repository check resultsThe repository checks are well-implemented but could be optimized by caching the results since repository status rarely changes during test runs.
+let isGitRepo = null; +let isHgRepo = null; + function isInGitRepository() { + if (isGitRepo !== null) return isGitRepo; try { execSync('git rev-parse --is-inside-work-tree', { stdio: 'ignore' }); - return true; + return (isGitRepo = true); } catch (e) { - return false; + return (isGitRepo = false); } }app/client/packages/design-system/ads-old/src/FilePickerV2/index.tsx (1)
262-262: Consider using proper TypeScript types instead of 'any'.The type assertion to 'any' could be replaced with proper types from react-dnd.
- const files = (monitor.getItem() as any).files; + const item = monitor.getItem() as { files: FileList }; + const files = item.files;app/client/config/paths.js (4)
1-5: Remove redundant 'use strict' directive.The 'use strict' directive is unnecessary as JavaScript modules are automatically in strict mode.
-'use strict'; const path = require('path');🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
26-38: Consider TypeScript-first approach for extensions.The module extensions are comprehensive, but consider reordering to prioritize TypeScript extensions for better TypeScript support.
const moduleFileExtensions = [ - 'web.mjs', - 'mjs', - 'web.js', - 'js', 'web.ts', 'ts', 'web.tsx', 'tsx', + 'web.mjs', + 'mjs', + 'web.js', + 'js', 'json', 'web.jsx', 'jsx', ];
40-51: Add error handling for module resolution.Consider adding error handling for cases where the file doesn't exist even with the .js extension.
const resolveModule = (resolveFn, filePath) => { const extension = moduleFileExtensions.find(extension => fs.existsSync(resolveFn(`${filePath}.${extension}`)) ); if (extension) { return resolveFn(`${filePath}.${extension}`); } + const jsPath = `${filePath}.js`; + if (!fs.existsSync(resolveFn(jsPath))) { + throw new Error(`Could not resolve module ${filePath} with any known extension`); + } return resolveFn(`${filePath}.js`); };
75-77: Remove unnecessary blank lines.Remove the extra blank lines before the moduleFileExtensions export.
}; - - module.exports.moduleFileExtensions = moduleFileExtensions;app/client/config/webpackDevServer.config.js (1)
1-1: Remove the redundant "use strict" directive.
Modern JavaScript modules are already in strict mode by default.- "use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
app/client/scripts/build.js (2)
1-1: Remove the redundant "use strict" directive.
No need for it in modern JS modules.- 'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
128-132: Optional chaining can simplify this check.
Using optional chaining can succinctly handle null-ish values for err.- if (err && err.message) { + if (err?.message) { console.log(err.message); }🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/config/webpack.config.js (1)
1-1: Remove the redundant "use strict" directive.
ES modules automatically apply strict mode.- "use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
app/client/config/webpack/persistentCache/createEnvironmentHash.js (2)
1-1: Remove redundant 'use strict' directiveModules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
4-9: Consider using SHA-256 instead of MD5While MD5 is sufficient for cache busting, SHA-256 is a more modern and secure hashing algorithm.
- const hash = createHash('md5'); + const hash = createHash('sha256');app/client/config/jest/babelTransform.js (1)
1-1: Remove redundant 'use strict' directiveModules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
app/client/src/react-app-env.d.ts (1)
44-53: Consider adding aria-label prop type for SVGFor better accessibility support.
export const ReactComponent: React.FunctionComponent< - React.SVGProps<SVGSVGElement> & { title?: string } + React.SVGProps<SVGSVGElement> & { title?: string; 'aria-label'?: string } >;app/client/config/getHttpsConfig.js (2)
1-1: Remove redundant 'use strict' directiveModules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
11-32: Add certificate expiration checkThe validation should also check if the certificate is expired.
function validateKeyAndCerts({ cert, key, keyFile, crtFile }) { let encrypted; + try { + const certObj = new crypto.X509Certificate(cert); + const now = new Date(); + if (now > certObj.validTo) { + throw new Error(`The certificate "${chalk.yellow(crtFile)}" has expired.`); + } + } catch (err) { + if (err.message.includes('expired')) { + throw err; + } + // Ignore other X509Certificate errors and continue with existing checks + } try { // Rest of the validation...app/client/config/modules.js (2)
14-50: Add TypeScript type definitions for better type safety.Consider adding TypeScript type definitions for the options parameter and return type to improve code maintainability.
-function getAdditionalModulePaths(options = {}) { +interface ModuleOptions { + baseUrl?: string; +} + +function getAdditionalModulePaths(options: ModuleOptions = {}): string[] | null {
94-132: Enhance error handling for configuration reading.Add error handling for the TypeScript configuration reading process to prevent potential runtime errors.
if (hasTsConfig) { const ts = require(resolve.sync('typescript', { basedir: paths.appNodeModules, })); - config = ts.readConfigFile(paths.appTsConfig, ts.sys.readFile).config; + const result = ts.readConfigFile(paths.appTsConfig, ts.sys.readFile); + if (result.error) { + throw new Error(`Failed to read tsconfig.json: ${result.error.messageText}`); + } + config = result.config; } else if (hasJsConfig) {app/client/scripts/start.js (1)
47-48: Consider using environment variables for server configuration.The default port and host are hardcoded. Consider moving these to environment variables with fallbacks.
-const DEFAULT_PORT = parseInt(process.env.PORT, 10) || 3000; -const HOST = process.env.HOST || '0.0.0.0'; +const DEFAULT_PORT = parseInt(process.env.REACT_APP_DEFAULT_PORT || process.env.PORT, 10) || 3000; +const HOST = process.env.REACT_APP_HOST || process.env.HOST || '0.0.0.0';app/client/package.json (1)
430-431: Consider extending from additional ESLint configurations.The current ESLint configuration only extends from "react-app". Consider adding additional presets for better code quality enforcement.
"eslintConfig": { - "extends": "react-app" + "extends": [ + "react-app", + "plugin:react-hooks/recommended", + "plugin:jsx-a11y/recommended" + ] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
app/client/build.sh(1 hunks)app/client/config/env.js(1 hunks)app/client/config/getHttpsConfig.js(1 hunks)app/client/config/jest/babelTransform.js(1 hunks)app/client/config/jest/cssTransform.js(1 hunks)app/client/config/jest/fileTransform.js(1 hunks)app/client/config/modules.js(1 hunks)app/client/config/paths.js(1 hunks)app/client/config/webpack.config.js(1 hunks)app/client/config/webpack/persistentCache/createEnvironmentHash.js(1 hunks)app/client/config/webpackDevServer.config.js(1 hunks)app/client/knip.json(0 hunks)app/client/package.json(12 hunks)app/client/packages/design-system/ads-old/src/FilePickerV2/index.tsx(2 hunks)app/client/scripts/build.js(1 hunks)app/client/scripts/start.js(1 hunks)app/client/scripts/test.js(1 hunks)app/client/src/components/editorComponents/HighlightedCode/index.tsx(1 hunks)app/client/src/react-app-env.d.ts(1 hunks)app/client/src/serviceWorker.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/knip.json
✅ Files skipped from review due to trivial changes (2)
- app/client/src/serviceWorker.ts
- app/client/src/components/editorComponents/HighlightedCode/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/config/webpack/persistentCache/createEnvironmentHash.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/scripts/test.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/jest/fileTransform.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/paths.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/getHttpsConfig.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/env.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/webpackDevServer.config.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/modules.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/scripts/start.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/scripts/build.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/config/jest/babelTransform.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/webpack.config.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/jest/cssTransform.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (20)
app/client/config/jest/cssTransform.js (1)
6-14: LGTM! Clean implementation of the Jest CSS transformer.The transformer correctly implements both required methods:
process(): Returns an empty module for CSS importsgetCacheKey(): Returns a static key since the transformation is constantapp/client/config/jest/fileTransform.js (1)
13-36: LGTM! Well-structured SVG transformation logic.The SVG handling follows best practices:
- Proper component naming using PascalCase
- Correct implementation of ref forwarding
- Maintains SVG props through Object.assign
app/client/scripts/test.js (4)
8-13: LGTM! Robust error handling implementation.Good practice to crash on unhandled rejections during tests rather than allowing silent failures.
40-49: LGTM! Smart watch mode configuration.Good implementation of watch mode selection based on CI environment and source control status.
52-52: LGTM! Clean test execution.Simple and effective Jest execution with properly processed arguments.
15-20: Verify environment configuration file existsThe implementation looks good, but let's verify the referenced environment configuration file.
✅ Verification successful
Environment configuration file verified ✓
The referenced environment configuration file exists at the expected location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the environment configuration file exists fd -t f "env.js" "app/client/config"Length of output: 59
app/client/packages/design-system/ads-old/src/FilePickerV2/index.tsx (1)
5-5: Verify react-dnd-html5-backend version compatibility.The change from default to named import suggests a breaking change in the package. Ensure the package.json specifies a compatible version.
✅ Verification successful
Import change is correct for react-dnd-html5-backend v14.x
The named import
{ HTML5Backend, NativeTypes }matches the required syntax for version 14.1.0 which is specified in the project's dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for react-dnd-html5-backend version rg '"react-dnd-html5-backend"' -A 1 package.jsonLength of output: 103
Script:
#!/bin/bash # Find relevant package.json files fd package.json --type f --full-path app/client/packages/design-system # Search for react-dnd-html5-backend in yarn.lock or package-lock.json fd "yarn.lock|package-lock.json" --type f --exec rg "react-dnd-html5-backend" -A 3 {}Length of output: 1014
app/client/config/paths.js (3)
7-10: LGTM! Robust symlink resolution implementation.The symlink resolution implementation follows best practices and includes proper documentation referencing the related CRA issue.
54-73: Consider validating critical paths on startup.Consider adding validation for critical paths (like appSrc, appPublic) to fail fast if they don't exist.
18-22: Consider adding validation for public URL configuration.While the implementation is correct, consider adding validation for the PUBLIC_URL environment variable and homepage field to prevent potential runtime issues.
app/client/config/webpackDevServer.config.js (1)
16-125: Configuration looks well-structured.
All essential dev server settings (proxies, CORS headers, etc.) appear correctly configured for the new setup post-CRA ejection. Nice work!app/client/config/webpack.config.js (1)
2-917: Overall configuration is comprehensive.
The setup handles various asset types, supports advanced optimizations, and aligns well with the ejection from CRA.app/client/config/jest/babelTransform.js (2)
5-16: LGTM! Well-structured JSX runtime detectionGood use of IIFE and environment variable check for JSX runtime detection.
18-29: LGTM! Clean Babel configurationConfiguration is properly structured with clear options for JSX runtime and disabled config file loading.
app/client/src/react-app-env.d.ts (1)
2-7: LGTM! Well-typed environment variablesClear and strict typing for NODE_ENV and PUBLIC_URL.
app/client/config/getHttpsConfig.js (1)
48-64: LGTM! Clean HTTPS configurationGood environment variable handling and path resolution.
app/client/build.sh (1)
21-21: LGTM! Build script successfully updated for CRA ejection.The script correctly maintains the memory allocation while switching from craco to direct node execution.
app/client/package.json (3)
19-19: Scripts have been updated to use direct node commands.The migration from CRACO to direct webpack configuration looks correct. The memory allocation (
--max_old_space_size) is properly set for build commands.Also applies to: 22-23, 26-27
274-278: Verify webpack configuration for production builds.The webpack-related packages are at the correct versions, but ensure the webpack configuration properly handles:
- Code splitting
- Tree shaking
- Production optimizations
✅ Verification successful
Webpack configuration verified and properly optimized ✅
The configuration implements all required optimizations:
- Code splitting with smart chunk strategies
- Tree shaking enabled for production builds
- Comprehensive production optimizations including minification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if webpack config exists and contains production optimizations fd -t f -e js webpack.config.js -x grep -l "optimization" {} \;Length of output: 104
Script:
#!/bin/bash # Examine webpack config for specific optimization settings cat ./app/client/config/webpack.config.js | grep -A 20 "optimization"Length of output: 1818
Script:
#!/bin/bash # Get a broader view of webpack configuration cat ./app/client/config/webpack.config.js | grep -A 30 "splitChunks\|module.exports"Length of output: 2633
Script:
#!/bin/bash # Check for tree shaking and module configurations cat ./app/client/config/webpack.config.js | grep -A 10 "sideEffects\|usedExports\|module: {"Length of output: 1271
141-142: Review environment variable handling.The addition of
dotenvand related packages is correct for environment variable management post-CRA ejection. Ensure that all environment variables are properly documented.Also applies to: 145-148
✅ Verification successful
Environment variables are properly documented ✅
The codebase includes comprehensive environment variable documentation in
.env.examplefiles, with clear descriptions and grouping by functionality. The addition ofdotenvpackages is justified and well-implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for environment variable documentation fd -t f "README.md" -x grep -l "Environment Variables\|ENV\|\.env" {} \;Length of output: 139
Script:
#!/bin/bash # Check content of README files and look for env-related files echo "=== README Contents ===" cat ./deploy/docker/README.md ./app/client/packages/icons/README.md echo -e "\n=== Env Example Files ===" fd -t f "^\.env" -x echo "=== {} ===" \; -x cat {} \; echo -e "\n=== Environment Variable Usage ===" rg "process\.env\." -A 2Length of output: 34559
Script:
#!/bin/bash # Search for .env example files and their contents echo "=== Environment Example Files ===" fd -H -t f "^\.env" -x echo "=== {} ===" \; -x cat {} \;Length of output: 1545
…o chore/eject-cra-retry-3
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/client/config/modules.js (1)
79-94:⚠️ Potential issueAdd missing return statement for non-root paths.
The function doesn't return anything when the path is not relative to appPath.
Apply this diff:
function getJestAliases(options = {}) { const baseUrl = options.baseUrl; if (!baseUrl) { return {}; } const baseUrlResolved = path.resolve(paths.appPath, baseUrl); if (path.relative(paths.appPath, baseUrlResolved) === '') { return { '^src/(.*)$': '<rootDir>/src/$1', }; } + return {}; }
🧹 Nitpick comments (3)
app/client/config/modules.js (2)
95-135: Add error handling for config loading.The config loading could fail if the files are malformed or inaccessible.
Add try-catch blocks:
if (hasTsConfig) { + try { const ts = require(resolve.sync('typescript', { basedir: paths.appNodeModules, })); config = ts.readConfigFile(paths.appTsConfig, ts.sys.readFile).config; + } catch (error) { + throw new Error(`Failed to load tsconfig.json: ${error.message}`); + } } else if (hasJsConfig) { + try { config = require(paths.appJsConfig); + } catch (error) { + throw new Error(`Failed to load jsconfig.json: ${error.message}`); + } }
1-1: Remove redundant 'use strict' directive.ES modules are automatically in strict mode.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
app/client/config/env.js (1)
63-102: Add validation for publicUrl parameter.The function should validate the publicUrl parameter to ensure it's a valid URL or path.
Add validation:
function getClientEnvironment(publicUrl) { + if (typeof publicUrl !== 'string') { + throw new TypeError('publicUrl must be a string'); + } const raw = Object.keys(process.env)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/config/env.js(1 hunks)app/client/config/jest/cssTransform.js(1 hunks)app/client/config/modules.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/config/jest/cssTransform.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/config/modules.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/env.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (4)
app/client/config/modules.js (1)
57-72: LGTM!The webpack aliases configuration is correctly implemented.
app/client/config/env.js (3)
85-90: Fast Refresh is deliberately disabled, impacting development experience.The configuration hardcodes Fast Refresh instead of using the environment variable.
#!/bin/bash # Check if Fast Refresh is enabled in the original CRA configuration rg -l "FAST_REFRESH.*=.*false" "app/client"
43-57: LGTM!The NODE_PATH configuration is well-implemented with proper path filtering and resolution.
1-1: Remove redundant 'use strict' directive.ES modules are automatically in strict mode.
-"use strict";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
…o chore/eject-cra-retry-3
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/client/config/modules.js (2)
1-8: Remove redundant 'use strict' directiveJavaScript modules are automatically in strict mode, making this directive unnecessary.
-"use strict"; const fs = require("fs");🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
111-117: Add error handling for TypeScript config readingThe TypeScript configuration reading should be wrapped in a try-catch block to handle potential parsing errors gracefully.
- const ts = require( - resolve.sync("typescript", { - basedir: paths.appNodeModules, - }), - ); - config = ts.readConfigFile(paths.appTsConfig, ts.sys.readFile).config; + try { + const ts = require( + resolve.sync("typescript", { + basedir: paths.appNodeModules, + }), + ); + config = ts.readConfigFile(paths.appTsConfig, ts.sys.readFile).config; + } catch (error) { + throw new Error( + chalk.red.bold( + `Failed to read TypeScript configuration: ${error.message}`, + ), + ); + }app/client/config/webpack.config.js (2)
1-2: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary as ES modules are automatically in strict mode.
-"use strict"; -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
38-41: Extract airgap build directory name as a constantThe build directory name 'build_airgap' is hardcoded in multiple places. Consider extracting it as a constant for better maintainability.
+const AIRGAP_BUILD_DIR = 'build_airgap'; + const isAirgap = process.env.REACT_APP_AIRGAP_ENABLED === "true"; if (isAirgap) { - paths.appBuild = path.resolve("build_airgap"); + paths.appBuild = path.resolve(AIRGAP_BUILD_DIR); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/config/modules.js(1 hunks)app/client/config/webpack.config.js(1 hunks)app/client/package.json(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/config/webpack.config.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
app/client/config/modules.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/config/modules.js (3)
14-49: LGTM! Clean implementation of module path resolutionThe function handles all edge cases appropriately and maintains consistent return types (null or array).
56-71: LGTM! Proper handling of webpack aliasesThe function correctly handles all cases and includes the previously missing return statement.
78-92:⚠️ Potential issueAdd missing return statement for non-root paths
The function needs a default return for when the path doesn't resolve to appPath.
}; } + return {}; }Likely invalid or redundant comment.
app/client/config/webpack.config.js (2)
304-342: LGTM: Efficient icon code-splitting strategyThe icon splitting configuration is well-implemented, separating icons into different chunks based on their source and usage patterns. This optimization helps reduce the initial bundle size.
805-805: Review the service worker cache size limitThe maximum file size to cache (11MB) seems unusually high. Consider if this large cache size is necessary and its impact on performance.
✅ Verification successful
The 11MB per-file cache limit is appropriate
The limit applies per-file (not total cache) and includes safeguards:
- Excludes source maps and manifest files
- Only caches production assets with content hashes
- Prevents caching extremely large files while allowing reasonable chunk sizes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the size of the largest JS chunks in the build find build -name "*.js" -type f -exec du -h {} + | sort -rh | head -n 5Length of output: 116
Script:
#!/bin/bash # Check for bundle size related configurations and documentation echo "=== Searching for bundle size configurations ===" rg -i "maxSize|bundleSize|fileSize|chunkSize" app/client/config/webpack.config.js echo -e "\n=== Checking for documentation about bundle sizes ===" fd README.md --exec cat {} echo -e "\n=== Looking for other service worker configurations ===" rg -i "maximumFileSizeToCacheInBytes|workbox" --type jsLength of output: 69896
Script:
#!/bin/bash # Search for workbox and service worker configuration echo "=== Searching for workbox configuration ===" rg -A 5 -B 5 "workbox|GenerateSW|InjectManifest" app/client/config/webpack.config.js echo -e "\n=== Looking for service worker implementation ===" fd "service-worker" --type f --exec cat {}Length of output: 2629
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13047145425. |
|
Deploy-Preview-URL: https://ce-38890.dp.appsmith.com |
| "workspaces": [ | ||
| "packages/**", | ||
| "!packages/**/build" | ||
| ], | ||
| "scripts": { | ||
| "analyze": "yarn cra-bundle-analyzer", | ||
| "start": "BROWSER=none REACT_APP_ENVIRONMENT=DEVELOPMENT REACT_APP_CLIENT_LOG_LEVEL=debug HOST=dev.appsmith.com craco --max_old_space_size=4096 start", | ||
| "start": "BROWSER=none REACT_APP_ENVIRONMENT=DEVELOPMENT REACT_APP_CLIENT_LOG_LEVEL=debug node --max_old_space_size=4096 scripts/start.js", |
There was a problem hiding this comment.
nit: I know it's not directly related, but it would be convenient if we also had a command to start the app without type checking, as it improves performance and reduces memory consumption.
Something along the lines:
"start-no-ts": "export ENABLE_TYPE_CHECKING=false && yarn start"
There was a problem hiding this comment.
Great job. It wasn't so easy to make it all work. 👍
I have one more nuance that I would like to draw your attention to. Given the changes in this PR, I think we should delete the current build configuration files.
- ci-build.sh
- craco.build.config.js
- craco.common.config.js
- craco.dev.config.js
| @@ -18,6 +18,6 @@ export REACT_APP_SENTRY_RELEASE=$GIT_SHA | |||
| export REACT_APP_CLIENT_LOG_LEVEL=ERROR | |||
| # Disable CRA built-in ESLint checks since we have our own config and a separate step for this | |||
| export DISABLE_ESLINT_PLUGIN=true | |||
| craco --max-old-space-size=8192 build --config craco.build.config.js | |||
| node --max-old-space-size=8192 scripts/build.js | |||
There was a problem hiding this comment.
@riodeuno could you please check how all this will work in EE repo?
There was a problem hiding this comment.
Are we sure we need files from the jest folder? I have not found any imports or places to use them. Could you double check it?
| const WorkboxWebpackPlugin = require("workbox-webpack-plugin"); | ||
| const ModuleScopePlugin = require("react-dev-utils/ModuleScopePlugin"); | ||
| const getCSSModuleLocalIdent = require("react-dev-utils/getCSSModuleLocalIdent"); | ||
| const ESLintPlugin = require("eslint-webpack-plugin"); |
There was a problem hiding this comment.
Do we really need to use ESLintPlugin? In the current version of the build, we don't use it. The lint rules are currently being checked during development in IDE, as well as a pre-commit hook when committing, and of course we check all the rules in CI. I would suggest removing the use of this plugin. Everything will work faster this way.
| const cssRegex = /\.css$/; | ||
| const cssModuleRegex = /\.module\.css$/; | ||
| const sassRegex = /\.(scss|sass)$/; | ||
| const sassModuleRegex = /\.module\.(scss|sass)$/; |
There was a problem hiding this comment.
Do we even need rules for SASS? We don't use sass or scss in the projects. Knowing this, could we simplify the configuration?
| const createEnvironmentHash = require("./webpack/persistentCache/createEnvironmentHash"); | ||
|
|
||
| // Source maps are resource heavy and can cause out of memory issue for large source files. | ||
| const shouldUseSourceMap = process.env.GENERATE_SOURCEMAP !== "false"; |
There was a problem hiding this comment.
Why do we need this variable? As far as I understand, in the current build we are creating source maps only for the prod build. Could we do the same here?
| "build-local": "craco --max-old-space-size=4096 build --config craco.build.config.js", | ||
| "build-staging": "REACT_APP_ENVIRONMENT=STAGING craco --max-old-space-size=4096 build --config craco.build.config.js", | ||
| "build-local": "node --max-old-space-size=4096 scripts/build.js", | ||
| "build-staging": "REACT_APP_ENVIRONMENT=STAGING node --max-old-space-size=4096 scripts/build.js ", |
There was a problem hiding this comment.
Do we use build-local and build-staging scripts somewhere? Why are they needed? Also, judging by value of -max-old-space-size these scripts should not work at all. Maybe it's worth deleting them?
| "eject": "react-scripts eject", | ||
| "start-prod": "REACT_APP_ENVIRONMENT=PRODUCTION craco start", | ||
| "cytest": "REACT_APP_TESTING=TESTING REACT_APP_ENVIRONMENT=DEVELOPMENT craco start & ./node_modules/.bin/cypress open", | ||
| "start-prod": "REACT_APP_ENVIRONMENT=PRODUCTION node scripts/start.js", |
| @@ -168,17 +190,25 @@ | |||
| "path-to-regexp": "^6.3.0", | |||
| "pluralize": "^8.0.0", | |||
| "popper.js": "^1.15.0", | |||
| "postcss": "^8.4.4", | |||
There was a problem hiding this comment.
Could you double-check the dependency lists? All dependencies from this PR should be specified in devDependencies. Also, many of them are already presented in the project and with more recent versions.
| @@ -386,5 +426,8 @@ | |||
| "@blueprintjs/icons": "3.22.0", | |||
| "@types/react": "^17.0.2", | |||
| "postcss": "8.4.31" | |||
| }, | |||
| "eslintConfig": { | |||
| @@ -0,0 +1,52 @@ | |||
| 'use strict'; | |||
|
|
|||
| // Do this as the first thing so that any code reading it knows the right env. | |||
There was a problem hiding this comment.
Why do we need this file? I don't see that we used it in scripts anywhere.
Description
Fixes #38903
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13046167970
Commit: 57c1b61
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 30 Jan 2025 06:31:29 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
Configuration Changes
Development Tools
Performance
Testing
Bug Fixes
No user-facing features were directly introduced in these changes.