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

chore: Eject CRA #38890

Open
wants to merge 16 commits into
base: release
Choose a base branch
from
Open

chore: Eject CRA #38890

wants to merge 16 commits into from

Conversation

riodeuno
Copy link
Contributor

@riodeuno riodeuno commented Jan 29, 2025

Description

  • Eject the application from create react app
  • Remove references to craco in the scripts and package.json dependencies
  • Port craco changes to webpack configuration

Fixes #38903

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Caution

🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13257060742
Commit: 6502ee6
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/BugTests/GitBugs_Spec.ts
  2. cypress/e2e/Regression/ClientSide/Git/GitDiscardChange/DiscardChanges_spec.js
  3. cypress/e2e/Regression/ClientSide/Git/GitWithJSLibrary/GitwithCustomJSLibrary_spec.js
  4. cypress/e2e/Regression/ServerSide/QueryPane/Mongo1_spec.ts
List of identified flaky tests.
Tue, 11 Feb 2025 08:59:27 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Build & Deployment

    • Transitioned to a streamlined, Node.js–based build and startup process for more efficient and reliable releases.
    • Enhanced environment and asset management, contributing to improved load times.
  • Testing & Quality

    • Upgraded testing workflows to handle assets and styles more robustly, ensuring higher overall release quality.
    • Introduced custom Jest transformers for handling CSS and file imports in tests.
  • Performance & Stability

    • Optimized bundling and error handling to deliver a smoother and more consistent user experience.
    • Improved HTTPS configuration management for secure connections.

Copy link
Contributor

coderabbitai bot commented Jan 29, 2025

Walkthrough

This pull request replaces the craco-based build and development commands with direct Node.js script invocations. It introduces new modules for managing environment variables, HTTPS configuration, module alias resolution, and environment hashing. Additionally, custom Jest transformers, updated webpack configurations, and revised type definitions for assets have been incorporated. Obsolete craco configuration files have been removed, and various package scripts and dependencies have been updated to reflect the ejected setup from CRA.

Changes

File(s) Change Summary
app/client/build.sh Updated build command from craco ... to node scripts/build.js with consistent memory settings.
app/client/config/env.js Added module to load and expand dotenv files and configure environment variables for the client.
app/client/config/getHttpsConfig.js Introduced HTTPS configuration with functions to validate keys/certificates and read env files.
app/client/config/jest/{babelTransform.js, cssTransform.js, fileTransform.js} Added custom Jest transformers for Babel, CSS, and file (SVG) handling.
app/client/config/modules.js Added module functions to resolve additional module paths and configure webpack/jest aliases based on baseUrl.
app/client/config/paths.js Created new configuration for resolving application paths and module file extensions.
app/client/config/webpack.config.js Introduced comprehensive Webpack configuration for production and development builds with various optimizations.
app/client/config/webpackDevServer.config.js Established new Webpack Dev Server configuration with middleware, proxy, and HTTPS support.
app/client/knip.json Removed craco-alias and craco-babel-loader from the ignoreDependencies list.
app/client/package.json Updated scripts to replace craco commands with Node.js scripts; modified/add dependencies accordingly.
app/client/packages/design-system/ads-old/src/FilePickerV2/index.tsx Adjusted import of HTML5Backend and applied type casting on dropped files for TypeScript compatibility.
app/client/scripts/{build.js, start.js, test.js} Added new build, start, and test scripts implementing Node-based build and development workflows.
app/client/src/components/editorComponents/HighlightedCode/index.tsx Updated configuration file reference comment from craco to webpack.config.js.
app/client/src/react-app-env.d.ts Revised type definitions: removed react-scripts and added environment and asset module declarations.
app/client/{craco.build.config.js, craco.common.config.js, craco.dev.config.js} Removed obsolete craco configuration files.
app/client/config/createEnvironmentHash.js Added a module that creates an MD5 hash from the environment configuration.

Sequence Diagram(s)

sequenceDiagram
  participant Dev as Developer
  participant BS as Build Script (build.js)
  participant WP as Webpack
  participant FS as File System
  Dev->>BS: Trigger build process
  BS->>FS: Validate required files & copy public assets
  BS->>WP: Execute Webpack build
  WP-->>BS: Return build results
  BS->>Dev: Log warnings/errors and output build stats
Loading
sequenceDiagram
  participant Dev as Developer
  participant SS as Start Script (start.js)
  participant WDS as Webpack Dev Server
  participant FS as File System
  Dev->>SS: Initiate development server
  SS->>FS: Verify critical files (HTML & entry JS)
  SS->>WDS: Create compiler, set proxy and middleware
  WDS-->>SS: Server startup confirmation
  SS->>Dev: Log server status and handle termination signals
Loading

Assessment against linked issues

Objective Addressed Explanation
Eject Appsmith FE from CRA (#38903)

Possibly related PRs

Suggested reviewers

  • sharat87
  • ApekshaBhosale
  • mohanarpit

Poem

In code we trust, the old now fades,
A new path is forged with Node's parade.
Craco steps aside in silent lore,
While scripts and configs boldly soar.
✨ Build and start, our code takes flight,
Debug and test deep into the night.
🚀 Happy coding to all, shining bright!

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Jan 29, 2025
@riodeuno riodeuno added the ok-to-test Required label for CI label Jan 29, 2025
@riodeuno riodeuno marked this pull request as ready for review January 29, 2025 11:17
@riodeuno riodeuno requested a review from KelvinOm as a code owner January 29, 2025 11:17
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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] peerDependencies

Length of output: 180


Script:

#!/bin/bash
# Check webpack version in package.json
rg '"webpack":' app/client/package.json

Length 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' directive

The '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 results

The 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' directive

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)


4-9: Consider using SHA-256 instead of MD5

While 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' directive

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/src/react-app-env.d.ts (1)

44-53: Consider adding aria-label prop type for SVG

For 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' directive

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)


11-32: Add certificate expiration check

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 754d3ef and e9f643e.

⛔ Files ignored due to path filters (1)
  • app/client/yarn.lock is 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 imports
  • getCacheKey(): Returns a static key since the transformation is constant
app/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 exists

The 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.json

Length 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 detection

Good use of IIFE and environment variable check for JSX runtime detection.


18-29: LGTM! Clean Babel configuration

Configuration 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 variables

Clear and strict typing for NODE_ENV and PUBLIC_URL.

app/client/config/getHttpsConfig.js (1)

48-64: LGTM! Clean HTTPS configuration

Good 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:

  1. Code splitting
  2. Tree shaking
  3. 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 dotenv and 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.example files, with clear descriptions and grouping by functionality. The addition of dotenv packages 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 2

Length 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

app/client/config/jest/cssTransform.js Outdated Show resolved Hide resolved
app/client/config/modules.js Outdated Show resolved Hide resolved
app/client/config/env.js Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
app/client/config/modules.js (1)

79-94: ⚠️ Potential issue

Add 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

📥 Commits

Reviewing files that changed from the base of the PR and between e9f643e and f538f4b.

📒 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)

app/client/config/modules.js Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (4)
app/client/config/modules.js (2)

1-8: Remove redundant 'use strict' directive

JavaScript 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 reading

The 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' directive

The '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 constant

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between f538f4b and 57c1b61.

📒 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 resolution

The function handles all edge cases appropriately and maintains consistent return types (null or array).


56-71: LGTM! Proper handling of webpack aliases

The function correctly handles all cases and includes the previously missing return statement.


78-92: ⚠️ Potential issue

Add 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 strategy

The 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 limit

The 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 5

Length 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 js

Length 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

app/client/config/webpack.config.js Show resolved Hide resolved
@riodeuno
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13047145425.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 38890.
recreate: .

Copy link

Deploy-Preview-URL: https://ce-38890.dp.appsmith.com

alex-golovanov
alex-golovanov previously approved these changes Feb 4, 2025
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alex-golovanov @KelvinOm I've disabled eslint plugin for the yarn start command. The other commands like build, etc, will still have it enabled.

The only problem with this approach is that if the IDE is not configured correctly to notify of eslint issues, developers will find it out during CI checks, which increase the time taken to identify issue and resolve the issue.

Noting that, in the craco configurations earlier, we had disabled them with no issues so far.

Copy link
Collaborator

@KelvinOm KelvinOm left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

@riodeuno could you please check how all this will work in EE repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KelvinOm Will do, let me create an EE PR as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current configuration, we're only generating source maps if the isEnvProduction boolean is true.

The GENERATE_SOURCEMAP, is used to globally stop the generation of sourcemaps by setting it to "false".

GENERATE_SOURCEMAP is a relic from CRA, and we can and should remove it.

However, #38890 (comment)

"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 ",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KelvinOm I'm going to leave the the scripts as-is for now. For the following reasons:

  1. I have no idea why they have been placed. So, instead of cramming more changes to this PR, once everything is merged, and we've gone through one release cycle, I can get back to this change
  2. There are no drawbacks to them being here for the time being.

app/client/package.json Outdated Show resolved Hide resolved
app/client/package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,52 @@
'use strict';

// Do this as the first thing so that any code reading it knows the right env.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this file? I don't see that we used it in scripts anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was generated by CRA. I'll take a look at it.

@riodeuno
Copy link
Contributor Author

@KelvinOm @alex-golovanov Thanks for reviewing this. I'll do the following

  1. Create an EE PR with the same changes and make sure that there are no issues with the CI
  2. Fix the merge conflicts in this PR and resolve all review comments.

@riodeuno
Copy link
Contributor Author

@KelvinOm I've gone through your review comments, and will be resolving many of them. However, although I agree with your suggestions, I will not be making all of the changes in this PR. This is because, I want to keep the changes to a minimum and focus on removing the dependency on CRA. Nevertheless, to make sure development is not effected, I will be making sure that the following happens:

  1. Source maps are only generated in production builds ( it is also used by the Faro plugin )
  2. Eslint checking does not occur during development.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (4)
app/client/config/webpack.config.js (4)

34-35: 🛠️ Refactor suggestion

Simplify source map configuration.

As discussed in the previous review, we should remove the GENERATE_SOURCEMAP environment variable and directly tie source map generation to production mode.

-// Source maps are resource heavy and can cause out of memory issue for large source files.
-const shouldUseSourceMap = process.env.GENERATE_SOURCEMAP !== "false";
+const shouldUseSourceMap = isEnvProduction;

81-82: 🛠️ Refactor suggestion

Remove unused SASS configuration.

As mentioned in the previous review, the project doesn't use SASS/SCSS. We should remove the unused configuration to simplify the webpack setup.

Remove the following:

  1. SASS regex constants (lines 81-82)
  2. SASS loader configurations (lines 593-631)

Also applies to: 593-631


849-894: 🛠️ Refactor suggestion

Make type checking configurable.

As discussed in the previous review, type checking should be configurable via an environment variable to improve build performance and prevent memory issues in the EE repo.

Add an environment variable check:

+const enableTypeChecking = process.env.ENABLE_TYPE_CHECKING !== "false";

-      new ForkTsCheckerWebpackPlugin({
+      enableTypeChecking && new ForkTsCheckerWebpackPlugin({

895-919: 🛠️ Refactor suggestion

Reconsider ESLint plugin usage.

As mentioned in the previous review, ESLint checks are already performed:

  • During development in IDE
  • Via pre-commit hooks
  • In CI

Consider removing the ESLint plugin to improve build performance.

-      !disableESLintPlugin &&
-        new ESLintPlugin({
-          // Plugin options
-          extensions: ["js", "mjs", "jsx", "ts", "tsx"],
-          formatter: require.resolve("react-dev-utils/eslintFormatter"),
-          eslintPath: require.resolve("eslint"),
-          failOnError: !(isEnvDevelopment && emitErrorsAsWarnings),
-          context: paths.appSrc,
-          cache: true,
-          cacheLocation: path.resolve(
-            paths.appNodeModules,
-            ".cache/.eslintcache",
-          ),
-          // ESLint class options
-          cwd: paths.appPath,
-          resolvePluginsRelativeTo: __dirname,
-          baseConfig: {
-            extends: [require.resolve("eslint-config-react-app/base")],
-            rules: {
-              ...(!hasJsxRuntime && {
-                "react/react-in-jsx-scope": "error",
-              }),
-            },
-          },
-        }),
🧹 Nitpick comments (3)
app/client/config/createEnvironmentHash.js (2)

1-2: Remove redundant 'use strict' directive.

The 'use strict' directive is unnecessary in ES modules as they are automatically in strict mode.

-'use strict';
const { createHash } = require('crypto');
🧰 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: Add input validation and consider hash algorithm choice.

While MD5 is sufficient for non-security purposes like caching, consider:

  1. Adding input validation for the env parameter
  2. Using a more robust hash algorithm if this is used for security purposes
 module.exports = env => {
+  if (!env || typeof env !== 'object') {
+    throw new TypeError('Environment must be a non-null object');
+  }
   const hash = createHash('md5');
   hash.update(JSON.stringify(env));

   return hash.digest('hex');
 };
app/client/config/webpack.config.js (1)

1-1: Remove redundant "use strict" directive.

The "use strict" directive is unnecessary as JavaScript 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)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70b60b6 and 7650e57.

📒 Files selected for processing (3)
  • app/client/config/createEnvironmentHash.js (1 hunks)
  • app/client/config/webpack.config.js (1 hunks)
  • app/client/package.json (13 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/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/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)

⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: perform-test / rts-build / build
  • GitHub Check: perform-test / client-build / client-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-build / client-build
  • GitHub Check: client-prettier / prettier-check
  • GitHub Check: chromatic-deployment
  • GitHub Check: chromatic-deployment
  • GitHub Check: storybook-tests

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
app/client/config/webpack.config.js (1)

1-1: Remove redundant 'use strict' directive.

The 'use strict' directive is unnecessary in ES modules as they are strict 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)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7650e57 and 6502ee6.

📒 Files selected for processing (2)
  • app/client/config/webpack.config.js (1 hunks)
  • app/client/package.json (13 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)

⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: perform-test / client-build / client-build
  • GitHub Check: perform-test / rts-build / build
  • GitHub Check: perform-test / server-build / server-unit-tests
  • GitHub Check: client-lint / client-lint
  • GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
  • GitHub Check: client-unit-tests / client-unit-tests
  • GitHub Check: client-prettier / prettier-check
  • GitHub Check: client-build / client-build
  • GitHub Check: chromatic-deployment
  • GitHub Check: chromatic-deployment
  • GitHub Check: storybook-tests
🔇 Additional comments (3)
app/client/config/webpack.config.js (3)

34-35: Simplify source map configuration.

The GENERATE_SOURCEMAP environment variable is a relic from CRA and can be removed since source maps are only generated in production.


79-82: Remove unused SASS configuration.

The SASS-related regex definitions can be removed as SASS is not used in the project.


894-918: Consider disabling ESLint plugin in webpack.

Since linting is already handled by:

  • IDE during development
  • Pre-commit hooks
  • CI pipeline

The ESLint webpack plugin can be safely removed to improve build performance.

@KelvinOm
Copy link
Collaborator

@KelvinOm I've gone through your review comments, and will be resolving many of them. However, although I agree with your suggestions, I will not be making all of the changes in this PR. This is because, I want to keep the changes to a minimum and focus on removing the dependency on CRA. Nevertheless, to make sure development is not effected, I will be making sure that the following happens:

  1. Source maps are only generated in production builds ( it is also used by the Faro plugin )
  2. Eslint checking does not occur during development.

I don't mind if we address my comments in separate PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Eject Appsmith FE from CRA
3 participants