fix: sourcemaps #39301
fix: sourcemaps #39301
Conversation
WalkthroughThe changes adjust the build and configuration process across several components. The build script no longer disables ESLint, ensuring linting runs during the build. The webpack configuration now uses the Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/config/webpack.config.js (1)
157-159: Standardize source map configuration across loaders.The source map generation is now consistently controlled by
REACT_APP_ENVIRONMENTacross all loaders, but the condition is repeated multiple times.Consider extracting the repeated condition into a constant:
+const shouldGenerateSourceMap = + process.env.REACT_APP_ENVIRONMENT === "PRODUCTION" || + process.env.REACT_APP_ENVIRONMENT === "DEVELOPMENT"; -sourceMap: - process.env.REACT_APP_ENVIRONMENT === "PRODUCTION" || - process.env.REACT_APP_ENVIRONMENT === "DEVELOPMENT", +sourceMap: shouldGenerateSourceMap,Also applies to: 168-170, 177-179, 194-197, 514-519, 534-536, 554-556
app/client/package.json (1)
1-424:❓ Verification inconclusive
Verify impact of removing bundle analysis capability.
The removal of the
analyzescript might affect the team's ability to monitor and optimize bundle sizes.Consider alternatives for bundle analysis:
- Use webpack-bundle-analyzer as a development dependency
- Configure source-map-explorer
- Use the built-in webpack stats
🏁 Script executed:
#!/bin/bash # Check if there are any existing bundle analysis tools rg -l "webpack-bundle-analyzer|source-map-explorer" package.jsonLength of output: 119
IMPORTANT: Verify Bundle Analysis Impact
The removal of the
analyzescript may affect our ability to monitor and optimize bundle sizes. Note that our initial search for bundle analysis tools yielded an error because the command targeted the wrong file path (rootpackage.jsoninstead ofapp/client/package.json).
- Action: Manually verify whether any bundle analysis references (e.g.,
webpack-bundle-analyzerorsource-map-explorer) remain inapp/client/package.jsonor elsewhere in the repository.- Suggestion: If bundle analysis is still required, consider:
- Adding
webpack-bundle-analyzeras a development dependency.- Configuring
source-map-explorer.- Utilizing the built-in webpack stats.
Please ensure that the intended alternatives are adequately set up to maintain our bundle size monitoring and optimization capabilities.
📜 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 (3)
app/client/build.sh(0 hunks)app/client/config/webpack.config.js(10 hunks)app/client/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/build.sh
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (4)
app/client/config/webpack.config.js (3)
400-400: Improved source map loader exclusion pattern.The exclusion pattern now correctly excludes both
@babel/runtimeandnode_modules, which will improve build performance.
637-647: Simplified FaroSourceMapUploaderPlugin condition.The plugin is now only enabled in the PRODUCTION environment, which aligns with best practices for source map uploading.
488-488:❓ Verification inconclusive
Verify babel-loader exclusion impact.
The addition of
exclude: /node_modules/to babel-loader might affect transpilation of node_modules that need to be processed.
🏁 Script executed:
#!/bin/bash # Search for any babel configuration files that might override this setting fd -t f -e babelrc -e babel.config.js -e babel.config.json # Search for any packages in node_modules that might need transpilation rg -l "browsers|targets" node_modules/*/package.jsonLength of output: 185
Review Verification Outcome: Babel-loader Exclusion Impact
The automated checks did not reveal any babel configuration files overriding the webpack setting, nor did they identify node_modules packages requiring transpilation. However, since the node_modules directory isn’t populated in the repository (and the grep for package.json in node_modules returned an error), please manually verify that no third‐party modules need Babel processing. If some modules require transpilation, consider adjusting or explicitly overriding the default exclusion.
Impacted snippet in
app/client/config/webpack.config.js(line 488):exclude: /node_modules/,app/client/package.json (1)
18-18: ESLint is now enabled during development.Removing
DISABLE_ESLINT_PLUGIN=truewill help catch issues earlier in the development cycle.
KelvinOm
force-pushed
the
fix/sourcemaps
branch
from
b0f22d8 to
88551e5
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13354778486. |
|
Deploy-Preview-URL: https://ce-39301.dp.appsmith.com |
| @@ -16,8 +16,6 @@ fi | |||
| # build cra app | |||
| 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 | |||
There was a problem hiding this comment.
This variable is no longer used.
| @@ -586,13 +601,6 @@ module.exports = function (webpackEnv) { | |||
| ].filter(Boolean), | |||
| }, | |||
| ignoreWarnings: [ | |||
| function ignoreSourcemapsloaderWarnings(warning) { | |||
There was a problem hiding this comment.
No longer ignore the warnings of the source maps, we just excluded the node modules from the generation.
https://github.com/appsmithorg/appsmith/pull/39301/files#diff-2826783e90c72e4335b396bb94e6783ea1a2f9c26af76de107dd9f5fe6d5191aR399
| @@ -15,8 +15,7 @@ | |||
| "!packages/**/build" | |||
| ], | |||
| "scripts": { | |||
| "analyze": "yarn cra-bundle-analyzer", | |||
There was a problem hiding this comment.
The script is no longer relevant
| devtool: | ||
| (process.env.REACT_APP_ENVIRONMENT === "PRODUCTION" && "source-map") || | ||
| (process.env.REACT_APP_ENVIRONMENT === "DEVELOPMENT" && | ||
| "cheap-module-source-map"), |
There was a problem hiding this comment.
Dev mode only works fine with cheap-module-source-map. Other types do not work correctly due to an error in zone.js and we see blank page. The same type of source maps was used earlier in react-scripts.
It will be necessary to figure out how to enable recommended type eval-source-map. With eval-source-map, the app is rebuilt 2 times faster(4s instead 8s).
Description
REACT_APP_ENVIRONMENT=PRODUCTIONandREACT_APP_ENVIRONMENT=DEVELOPMENTas before.DISABLE_ESLINT_PLUGINcra-bundle-analyzerpackage and related script.EE PR — https://github.com/appsmithorg/appsmith-ee/pull/6227
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13353994445
Commit: 88551e5
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/Datasources/Snowflake_Basic_Spec.ts
List of identified flaky tests.Sun, 16 Feb 2025 12:54:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit