chore: Eject CRA #39264
chore: Eject CRA #39264
Conversation
…o chore/eject-cra-retry-3
…o chore/eject-cra-retry-3
…o chore/eject-cra-retry-3
…o chore/eject-cra-retry-3
…o chore/eject-cra-retry-3
WalkthroughThis pull request updates the client application's build and configuration process. It adds the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BS as build.sh
participant B as build.js
participant FS as File System Util
participant W as Webpack
U->>BS: Execute build command
BS->>B: Run Node.js script with memory limit
B->>FS: Validate required files & load environment
B->>W: Generate production webpack configuration
W-->>B: Return build results
B-->>U: Log output and stats
sequenceDiagram
participant U as User
participant S as start.js
participant FC as File Checker
participant WDS as Webpack Dev Server
U->>S: Execute start command
S->>FC: Verify required files and env variables
S->>WDS: Launch development server
WDS-->>U: Serve app and open browser
Possibly related PRs
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
|
riodeuno
changed the title
github-actions
bot
added
the
skip-changelog
riodeuno
requested review from
alex-golovanov
and removed request for
sharat87
|
@alex-golovanov @KelvinOm This is the follow up PR with all of the remaining review comments from the Eject CRA PR , resolved. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13323381254. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
app/client/scripts/start.js (2)
1-1: Remove the redundant 'use strict' directive.JavaScript modules automatically run in strict mode, making this directive unnecessary.
- '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)
149-154: Consider optional chaining for conciseness.Instead of checking both
erranderr.message, you can rely on optional chaining:- if (err && err.message) { - console.log(err.message); - } + if (err?.message) { + console.log(err.message); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/config/webpackDevServer.config.js (1)
1-1: Remove the redundant 'use strict' directive.JavaScript modules automatically run in strict mode, so this directive is unnecessary.
- "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.Same rationale: modules default to 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)
128-132: Consider optional chaining for brevity.You can simplify the condition by using optional chaining:
- .catch(err => { - if (err && err.message) { - console.log(err.message); - } - process.exit(1); - }); + .catch(err => { + if (err?.message) { + console.log(err.message); + } + process.exit(1); + });🧰 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 (2)
1-1: Remove the “use strict” directive.Modules automatically run in strict mode, so this directive is redundant and can be safely deleted.
Apply this diff to remove it:
-"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)
82-790: Consider splitting large config for maintainability.This file contains a comprehensive configuration for multiple environments. Splitting it into dedicated development/production config files or extracting complex logic into reusable modules can improve maintainability and readability in the long run.
app/client/config/createEnvironmentHash.js (1)
1-1: Remove the “use strict” directive.This file is already in strict mode by default as it’s an ES module.
Apply this diff to remove it:
-'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/getHttpsConfig.js (1)
1-1: Remove redundant 'use strict' directive.The 'use strict' directive is unnecessary as 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/paths.js (2)
1-1: Remove redundant 'use strict' directive.The 'use strict' directive is unnecessary as 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)
75-77: Remove unnecessary empty lines.Multiple consecutive empty lines before the final export.
}; - - - module.exports.moduleFileExtensions = moduleFileExtensions;app/client/config/modules.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)
app/client/config/env.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
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
app/client/.babelrc(1 hunks)app/client/build.sh(1 hunks)app/client/config/createEnvironmentHash.js(1 hunks)app/client/config/env.js(1 hunks)app/client/config/getHttpsConfig.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/webpackDevServer.config.js(1 hunks)app/client/craco.build.config.js(0 hunks)app/client/craco.common.config.js(0 hunks)app/client/craco.dev.config.js(0 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/src/components/editorComponents/HighlightedCode/index.tsx(1 hunks)app/client/src/react-app-env.d.ts(1 hunks)
💤 Files with no reviewable changes (4)
- app/client/knip.json
- app/client/craco.common.config.js
- app/client/craco.dev.config.js
- app/client/craco.build.config.js
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/ads-old/src/FilePickerV2/index.tsx
🧰 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)
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/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/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/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)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
🔇 Additional comments (12)
app/client/config/createEnvironmentHash.js (1)
4-9: Looks good!The function correctly hashes the environment object using MD5.
app/client/.babelrc (1)
3-3:✅ Verification successful
Verify installation of the plugin.
Ensure
"babel-plugin-lodash"is present in your dependencies.Run the following script to confirm that it’s listed in your package.json or lock files:
🏁 Script executed:
#!/bin/bash rg '"babel-plugin-lodash"' -A 2Length of output: 374
Dependency Verified: "babel-plugin-lodash" is installed
The required plugin is present in our dependencies—confirmed by its appearance in both
app/client/package.json(version^3.3.4) andapp/client/knip.json. No further action is needed.app/client/src/react-app-env.d.ts (1)
1-69: LGTM! Well-structured type declarations.The type declarations are comprehensive and follow TypeScript best practices, including proper readonly modifiers and explicit type constraints.
app/client/config/getHttpsConfig.js (1)
11-64: LGTM! Robust HTTPS configuration implementation.The implementation includes proper certificate validation and clear error messages.
app/client/src/components/editorComponents/HighlightedCode/index.tsx (2)
33-33: LGTM! Updated configuration reference.Comment correctly reflects the move from craco to webpack configuration.
74-78: Verify multiline prop implementation.The multiline prop is only used to conditionally render the code block, but there's no implementation for the multiline case.
app/client/config/paths.js (1)
26-73: LGTM! Well-structured path resolution.The implementation provides comprehensive path resolution with proper handling of module extensions and public URL configuration.
app/client/config/modules.js (1)
94-134: Well-structured module resolution implementation.The getModules function effectively handles both TypeScript and JavaScript configurations, with proper error handling and dynamic TypeScript loading.
app/client/config/env.js (1)
63-102: Well-implemented environment variable handling.The getClientEnvironment function correctly:
- Filters REACT_APP_ prefixed variables
- Provides both raw and stringified versions
- Includes essential variables for development
app/client/build.sh (1)
21-21: Clean transition from craco to node script.The change maintains the memory allocation while removing the craco dependency.
app/client/package.json (2)
18-25: Clean script updates removing craco dependency.The script changes align with the PR objective of removing CRA dependencies while maintaining functionality.
178-179:❓ Verification inconclusive
Verify react-dnd upgrade impact.
The major version upgrade of react-dnd (9.x → 14.x) and react-dnd-html5-backend might require code changes.
🏁 Script executed:
#!/bin/bash # Search for react-dnd usage that might need updates rg -A 5 'useDrag|useDrop|DndProvider'Length of output: 18245
React-DnD Upgrade Verification
The grep search confirms that the primary react-dnd usage—especially in the FilePickerV2 component (via DndProvider and useDrop)—appears consistent with the new versions. However, given the substantial jump from version 9.x to 14.x, please double-check that all behaviors (e.g., drag and drop interactions) still function as expected during manual testing.
- File: app/client/package.json (Lines 178–179) shows the updated dependency versions.
- File: app/client/packages/design-system/ads-old/src/FilePickerV2/index.tsx demonstrates proper usage of DndProvider and useDrop.
- Although no immediate breaking changes were detected via search, ensure that all react-dnd-related functionality has been verified through thorough testing.
| 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", | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add missing return statement in getJestAliases.
The function is missing a return statement for the else case, which could lead to undefined being returned.
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 {};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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", | |
| }; | |
| } | |
| } | |
| 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 {}; | |
| } |
|
Deploy-Preview-URL: https://ce-39264.dp.appsmith.com |
| @@ -183,7 +166,7 @@ module.exports = function (webpackEnv) { | |||
| { | |||
| loader: require.resolve("resolve-url-loader"), | |||
| options: { | |||
| sourceMap: isEnvProduction ? shouldUseSourceMap : isEnvDevelopment, | |||
| sourceMap: isEnvProduction ? true : isEnvDevelopment, | |||
There was a problem hiding this comment.
I think it might just be like that.
| sourceMap: isEnvProduction ? true : isEnvDevelopment, | |
| sourceMap: isEnvProduction, |
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/13323376575
Commit: 8f206bd
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 14 Feb 2025 07:06:14 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit