-
Notifications
You must be signed in to change notification settings - Fork 714
Stage #9325
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
Stage #9325
Changes from all commits
a7bf068
3a805ad
70292d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| require('webpack'); | ||
| const webpack = require('webpack'); | ||
| //Polyfill Node.js core modules in Webpack. This module is only needed for webpack 5+. | ||
| const TerserPlugin = require('terser-webpack-plugin'); | ||
| //Polyfill Node.js core modules in Webpack. This module is only needed for webpack 5+. | ||
|
|
@@ -17,13 +17,20 @@ if (isCircleEnv) { | |
| } | ||
|
|
||
| module.exports = { | ||
| target: 'electron-renderer', | ||
| target: 'web', | ||
| resolve: { | ||
| mainFields: ['es2016', 'browser', 'module', 'main'] | ||
| mainFields: ['es2016', 'browser', 'module', 'main'], | ||
| conditionNames: ['browser', 'import', 'default'], | ||
| fallback: { | ||
| crypto: require.resolve('crypto-browserify'), | ||
| stream: require.resolve('stream-browserify'), | ||
| util: require.resolve('util/'), | ||
| buffer: require.resolve('buffer/'), | ||
| process: require.resolve('process/browser') | ||
| } | ||
| }, | ||
|
Comment on lines
+22
to
31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding browser-specific polyfills (crypto-browserify, stream-browserify, util, buffer, process/browser) to an Electron app's webpack configuration is problematic. Electron apps should use Node.js built-in modules directly, not browser polyfills. This configuration treats the app as if it needs to run in a browser, which conflicts with:
This polyfill configuration will:
Remove the fallback polyfill configuration or restore the 'electron-renderer' target. The conditionNames should also be electron-specific, not browser-specific. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/agent/config/custom-webpack.config.js
Line: 22:31
Comment:
Adding browser-specific polyfills (crypto-browserify, stream-browserify, util, buffer, process/browser) to an Electron app's webpack configuration is problematic. Electron apps should use Node.js built-in modules directly, not browser polyfills. This configuration treats the app as if it needs to run in a browser, which conflicts with:
1. The electron-renderer environment where native Node.js modules are available
2. The use of electron-related externals (electron, electron-log) which are not browser APIs
3. The app's actual Electron architecture
This polyfill configuration will:
- Increase bundle size unnecessarily
- Cause module resolution conflicts
- Potentially break native Electron functionality
Remove the fallback polyfill configuration or restore the 'electron-renderer' target. The conditionNames should also be electron-specific, not browser-specific.
How can I resolve this? If you propose a fix, please make it concise. |
||
| optimization: { | ||
| concatenateModules: false, | ||
| // for now let's disable minimize in CircleCI | ||
| minimize: !isCircleCI, | ||
| minimizer: [ | ||
| new TerserPlugin({ | ||
|
|
@@ -38,11 +45,20 @@ module.exports = { | |
| ] | ||
| }, | ||
| externals: { | ||
| 'electron-log': 'electron-log' | ||
| 'electron-log': 'electron-log', | ||
| electron: 'commonjs electron' | ||
| }, | ||
| plugins: [ | ||
| new NodePolyfillPlugin({ | ||
| excludeAliases: ['console'] | ||
| }), | ||
| new webpack.ProvidePlugin({ | ||
| process: 'process/browser', | ||
| Buffer: ['buffer', 'Buffer'] | ||
| }), | ||
| // Define global as window for browser compatibility | ||
| new webpack.DefinePlugin({ | ||
| global: 'globalThis' | ||
| }) | ||
| ], | ||
| output: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,9 +61,10 @@ | |||||||||||||||
| "build:lerna": "yarn lerna run --parallel build", | ||||||||||||||||
| "build:prod": "yarn build:package:all && concurrently --raw \"yarn build:api:prod\" \"yarn build:gauzy:prod\"", | ||||||||||||||||
| "build:gauzy": "yarn run postinstall.web && yarn run config:dev && yarn ng build gauzy", | ||||||||||||||||
| "build:gauzy:prod": "yarn run config:prod && yarn ng:prod build gauzy -c=production --prod --verbose", | ||||||||||||||||
| "build:gauzy:prod:docker": "yarn run config:prod && yarn ng:docker build gauzy -c=production --prod --verbose", | ||||||||||||||||
| "build:gauzy:prod:ci": "yarn run config:prod && yarn --frozen-lockfile --cache-folder ~/.cache/yarn ng:ci build gauzy -c=production --prod --verbose", | ||||||||||||||||
| "build:gauzy:prod": "yarn run config:prod && yarn ng:prod build gauzy -c=production --prod", | ||||||||||||||||
| "build:gauzy:prod:debug": "yarn run config:prod && yarn ng:prod build gauzy -c=production --prod --verbose", | ||||||||||||||||
| "build:gauzy:prod:docker": "yarn run config:prod && yarn ng:docker build gauzy -c=production --prod", | ||||||||||||||||
| "build:gauzy:prod:ci": "yarn run config:prod && yarn --frozen-lockfile --cache-folder ~/.cache/yarn ng:ci build gauzy -c=production --prod", | ||||||||||||||||
|
Comment on lines
+64
to
+67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: The build:package:gauzy scripts have been fundamentally changed. The old scripts built a comprehensive set of packages (constants, contracts, common, utils, config, plugin, auth, plugins:pre, core, plugins:post), but the new scripts now build only UI-related packages (constants, contracts, ui-config, ui-core, ui-auth, plugins:pre). This is a breaking change that removes critical backend packages from the build chain. The API build scripts (build:package:api) still reference packages like "common", "utils", "config", "plugin", and "core" which will no longer be built by build:package:gauzy. This will cause the API builds to fail due to missing dependencies. The new build scripts are incomplete and don't match the API requirements.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: package.json
Line: 64:67
Comment:
CRITICAL: The build:package:gauzy scripts have been fundamentally changed. The old scripts built a comprehensive set of packages (constants, contracts, common, utils, config, plugin, auth, plugins:pre, core, plugins:post), but the new scripts now build only UI-related packages (constants, contracts, ui-config, ui-core, ui-auth, plugins:pre).
This is a breaking change that removes critical backend packages from the build chain. The API build scripts (build:package:api) still reference packages like "common", "utils", "config", "plugin", and "core" which will no longer be built by build:package:gauzy. This will cause the API builds to fail due to missing dependencies.
The new build scripts are incomplete and don't match the API requirements.
```suggestion
"build:package:gauzy": "yarn run build:package:constants && yarn run build:package:contracts && yarn run build:package:common && yarn run build:package:utils && yarn run build:package:config && yarn run build:package:plugin && yarn run build:package:auth && yarn run build:package:plugins:pre && yarn run build:package:core && yarn run build:package:plugins:post",
"build:package:gauzy:prod": "yarn run build:package:constants:prod && yarn run build:package:contracts:prod && yarn run build:package:common:prod && yarn run build:package:utils:prod && yarn run build:package:config:prod && yarn run build:package:plugin:prod && yarn run build:package:auth:prod && yarn run build:package:plugins:pre:prod && yarn run build:package:core:prod && yarn run build:package:plugins:post:prod",
"build:package:gauzy:docker": "yarn run build:package:constants:docker && yarn run build:package:contracts:docker && yarn run build:package:common:docker && yarn run build:package:utils:docker && yarn run build:package:config:docker && yarn run build:package:plugin:docker && yarn run build:package:auth:docker && yarn run build:package:plugins:pre:docker && yarn run build:package:core:docker && yarn run build:package:plugins:post:docker",
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+64
to
+67
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent removal of If verbosity needs to be reduced for performance, it should be done consistently. Alternatively, if you want to keep the verbose option available, create a separate dedicated debug script instead of inconsistently removing it. Consider this for consistency across all prod build scripts. Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: package.json
Line: 64:67
Comment:
Inconsistent removal of `--verbose` flag: Line 64 removes `--verbose` from build:gauzy:prod but the flag was still present in build:gauzy:prod:debug (line 65). However, checking the git diff, the --verbose flag is removed from all three scripts (build:gauzy:prod, build:gauzy:prod:docker, build:gauzy:prod:ci) inconsistently. This breaks consistency in build output verbosity settings.
If verbosity needs to be reduced for performance, it should be done consistently. Alternatively, if you want to keep the verbose option available, create a separate dedicated debug script instead of inconsistently removing it.
Consider this for consistency across all prod build scripts.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||
| "build:api": "yarn ng build api --configuration=development", | ||||||||||||||||
| "build:api:prod": "yarn ng:prod build api --configuration=production", | ||||||||||||||||
| "build:api:prod:docker": "yarn ng:docker build api --configuration=production", | ||||||||||||||||
|
|
@@ -299,9 +300,9 @@ | |||||||||||||||
| "build:package:api": "yarn run build:package:constants && yarn run build:package:contracts && yarn run build:package:common && yarn run build:package:utils && yarn run build:package:config && yarn run build:package:plugin && yarn run build:package:auth && yarn run build:package:core && yarn run build:package:plugins:post && yarn run build:package:plugin:integration-wakatime", | ||||||||||||||||
| "build:package:api:prod": "yarn run build:package:constants:prod && yarn run build:package:contracts:prod && yarn run build:package:common:prod && yarn run build:package:utils:prod && yarn run build:package:config:prod && yarn run build:package:plugin:prod && yarn run build:package:auth:prod && yarn run build:package:core:prod && yarn run build:package:plugins:post:prod", | ||||||||||||||||
| "build:package:api:docker": "yarn run build:package:constants:docker && yarn run build:package:contracts:docker && yarn run build:package:common:docker && yarn run build:package:utils:docker && yarn run build:package:config:docker && yarn run build:package:plugin:docker && yarn run build:package:auth:docker && yarn run build:package:core:docker && yarn run build:package:plugins:post:docker", | ||||||||||||||||
| "build:package:gauzy": "yarn run build:package:constants && yarn run build:package:contracts && yarn run build:package:common && yarn run build:package:utils && yarn run build:package:config && yarn run build:package:plugin && yarn run build:package:auth && yarn run build:package:plugins:pre && yarn run build:package:core && yarn run build:package:plugins:post", | ||||||||||||||||
| "build:package:gauzy:prod": "yarn run build:package:constants:prod && yarn run build:package:contracts:prod && yarn run build:package:common:prod && yarn run build:package:utils:prod && yarn run build:package:config:prod && yarn run build:package:plugin:prod && yarn run build:package:auth:prod && yarn run build:package:plugins:pre:prod && yarn run build:package:core:prod && yarn run build:package:plugins:post:prod", | ||||||||||||||||
| "build:package:gauzy:docker": "yarn run build:package:constants:docker && yarn run build:package:contracts:docker && yarn run build:package:common:docker && yarn run build:package:utils:docker && yarn run build:package:config:docker && yarn run build:package:plugin:docker && yarn run build:package:auth:docker && yarn run build:package:plugins:pre:docker && yarn run build:package:core:docker && yarn run build:package:plugins:post:docker", | ||||||||||||||||
| "build:package:gauzy": "yarn run build:package:constants && yarn run build:package:contracts && yarn run build:package:ui-config && yarn run build:package:ui-core && yarn run build:package:ui-auth && yarn run build:package:plugins:pre", | ||||||||||||||||
| "build:package:gauzy:prod": "yarn run build:package:constants:prod && yarn run build:package:contracts:prod && yarn run build:package:ui-config:prod && yarn run build:package:ui-core:prod && yarn run build:package:ui-auth:prod && yarn run build:package:plugins:pre:prod", | ||||||||||||||||
| "build:package:gauzy:docker": "yarn run build:package:constants:docker && yarn run build:package:contracts:docker && yarn run build:package:ui-config:docker && yarn run build:package:ui-core:docker && yarn run build:package:ui-auth:docker && yarn run build:package:plugins:pre:docker", | ||||||||||||||||
| "build:package:desktop-activity": "cross-env NODE_ENV=development NODE_OPTIONS=--max-old-space-size=12288 yarn nx build desktop-activity", | ||||||||||||||||
| "build:package:desktop-activity:prod": "cross-env NODE_ENV=production NODE_OPTIONS=--max-old-space-size=12288 yarn nx build desktop-activity", | ||||||||||||||||
| "build:package:desktop-activity:docker": "cross-env NODE_ENV=production NODE_OPTIONS=--max-old-space-size=30000 yarn nx build desktop-activity", | ||||||||||||||||
|
|
@@ -643,6 +644,7 @@ | |||||||||||||||
| "lighthouse": "^6.3.0", | ||||||||||||||||
| "lint-staged": "^10.4.0", | ||||||||||||||||
| "ng-packagr": "19.2.2", | ||||||||||||||||
| "node-abi": "^3.78.0", | ||||||||||||||||
| "node-gyp": "^10.2.0", | ||||||||||||||||
| "npm-run-all": "^4.1.5", | ||||||||||||||||
| "nx": "21.1.0", | ||||||||||||||||
|
|
@@ -674,8 +676,7 @@ | |||||||||||||||
| "webpack-bundle-analyzer": "^4.10.2", | ||||||||||||||||
| "webpack-cli": "^6.0.1", | ||||||||||||||||
| "webpack-merge": "^6.0.1", | ||||||||||||||||
| "webpack-node-externals": "^3.0.0", | ||||||||||||||||
| "node-abi": "^3.78.0" | ||||||||||||||||
| "webpack-node-externals": "^3.0.0" | ||||||||||||||||
| }, | ||||||||||||||||
| "overrides": { | ||||||||||||||||
| "prebuild": { | ||||||||||||||||
|
|
@@ -685,9 +686,13 @@ | |||||||||||||||
| "locutus": "2.0.30" | ||||||||||||||||
| }, | ||||||||||||||||
| "electron-rebuild": { | ||||||||||||||||
| "node-abi": "3.78.0", | ||||||||||||||||
| "@electron/rebuild/node-abi": "3.78.0", | ||||||||||||||||
| "app-builder-lib/node-abi": "3.78.0" | ||||||||||||||||
| "node-abi": "3.78.0" | ||||||||||||||||
| }, | ||||||||||||||||
| "@electron/rebuild": { | ||||||||||||||||
| "node-abi": "3.78.0" | ||||||||||||||||
| }, | ||||||||||||||||
| "app-builder-lib": { | ||||||||||||||||
| "node-abi": "3.78.0" | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
686
to
696
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reorganized the Consider adding a comment explaining why @electron/rebuild and app-builder-lib need separate node-abi overrides, or consolidate them back into a single electron-rebuild override block with better documentation. Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: package.json
Line: 686:696
Comment:
Reorganized the `overrides` section to split electron-rebuild related node-abi configurations. While this change works, it creates scattered configuration that was previously centralized. The intent seems to be clarifying which dependencies need the node-abi override, but this reduces maintainability.
Consider adding a comment explaining why @electron/rebuild and app-builder-lib need separate node-abi overrides, or consolidate them back into a single electron-rebuild override block with better documentation.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||
| }, | ||||||||||||||||
| "engines": { | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| } | ||
| }, | ||
| "tags": [], | ||
| "implicitDependencies": [], | ||
| "targets": { | ||
| "build": { | ||
| "executor": "@nx/js:tsc", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| } | ||
| }, | ||
| "tags": [], | ||
| "implicitDependencies": [], | ||
| "targets": { | ||
| "build": { | ||
| "executor": "@nx/js:tsc", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| } | ||
| }, | ||
| "tags": [], | ||
| "implicitDependencies": [], | ||
| "targets": { | ||
| "build": { | ||
| "executor": "@nx/js:tsc", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| } | ||
| }, | ||
| "tags": [], | ||
| "implicitDependencies": [], | ||
| "targets": { | ||
| "build": { | ||
| "executor": "@nx/js:tsc", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRITICAL: Changing webpack target from 'electron-renderer' to 'web' will break the Electron application. The agent app is designed to run as an Electron renderer process, not as a web application. This change will cause the app to fail to load properly in Electron.
The 'electron-renderer' target is necessary for Electron apps to properly handle Node.js modules and browser APIs. Using 'web' target will cause module resolution failures and API incompatibilities.
Prompt To Fix With AI