korbit-ai[bot] commented on code in PR #33887:
URL: https://github.com/apache/superset/pull/33887#discussion_r2164451906
##########
superset-frontend/scripts/build.js:
##########
@@ -105,35 +77,19 @@ function getPackages(packagePattern, tsOnly = false) {
let scope = getPackages(glob);
-if (shouldLint) {
- run(`npm run eslint -- . --fix {packages,plugins}/${scope}/{src,test}`);
-}
-
-if (shouldCleanup) {
- // these modules will be installed by `npm link` but not useful for actual
build
- const dirtyModules = 'node_modules/@types/react,node_modules/@superset-ui';
- const cachePath =
`./node_modules/${scope}/{lib,esm,tsconfig.tsbuildinfo,${dirtyModules}}`;
- console.log(`\n>> Cleaning up ${cachePath}`);
- sync(cachePath);
-}
+console.log('--- Run babel --------');
+const babelCommand = `lerna exec --stream --concurrency 10 --scope ${scope}
+ -- babel ${BABEL_CONFIG} src --extensions ".ts,.tsx,.js,.jsx"
--copy-files`;
Review Comment:
### Hardcoded Babel Configuration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The babel command configuration is hardcoded within the script, making it
difficult to modify or reuse the configuration in other parts of the build
system.
###### Why this matters
Hard-coded configuration reduces flexibility and maintainability. If babel
configuration needs to be changed or reused, it would require modifying the
code in multiple places.
###### Suggested change ∙ *Feature Preview*
Extract babel configuration into a separate configuration object or file:
```javascript
const BABEL_SETTINGS = {
extensions: [".ts", ".tsx", ".js", ".jsx"],
concurrency: 10,
configFile: '../../babel.config.js'
};
const createBabelCommand = (scope) =>
`lerna exec --stream --concurrency ${BABEL_SETTINGS.concurrency} --scope
${scope} -- babel --config-file=${BABEL_SETTINGS.configFile} src --extensions
"${BABEL_SETTINGS.extensions.join(',')}" --copy-files`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d942e4-d5fb-450e-b96c-dd3ba3224f59/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d942e4-d5fb-450e-b96c-dd3ba3224f59?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d942e4-d5fb-450e-b96c-dd3ba3224f59?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d942e4-d5fb-450e-b96c-dd3ba3224f59?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d942e4-d5fb-450e-b96c-dd3ba3224f59)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9e563600-56ec-474e-9842-8644c7ab9a99 -->
[](9e563600-56ec-474e-9842-8644c7ab9a99)
##########
superset-frontend/scripts/build.js:
##########
@@ -105,35 +77,19 @@ function getPackages(packagePattern, tsOnly = false) {
let scope = getPackages(glob);
-if (shouldLint) {
- run(`npm run eslint -- . --fix {packages,plugins}/${scope}/{src,test}`);
-}
-
-if (shouldCleanup) {
- // these modules will be installed by `npm link` but not useful for actual
build
- const dirtyModules = 'node_modules/@types/react,node_modules/@superset-ui';
- const cachePath =
`./node_modules/${scope}/{lib,esm,tsconfig.tsbuildinfo,${dirtyModules}}`;
- console.log(`\n>> Cleaning up ${cachePath}`);
- sync(cachePath);
-}
+console.log('--- Run babel --------');
+const babelCommand = `lerna exec --stream --concurrency 10 --scope ${scope}
+ -- babel ${BABEL_CONFIG} src --extensions ".ts,.tsx,.js,.jsx"
--copy-files`;
+run(`${babelCommand} --out-dir lib`);
Review Comment:
### Hardcoded Concurrency Limit <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Babel transpilation processes are running with a fixed concurrency of 10
regardless of the system's capabilities.
###### Why this matters
On systems with more CPU cores, this limits parallel processing potential.
On systems with fewer cores, this could cause resource contention.
###### Suggested change ∙ *Feature Preview*
Make concurrency configurable based on system resources:
```js
const os = require('os');
const cpuCount = os.cpus().length;
const concurrency = Math.max(1, Math.min(cpuCount - 1, 10));
const babelCommand = `lerna exec --stream --concurrency ${concurrency}
--scope ${scope}
-- babel ${BABEL_CONFIG} src --extensions ".ts,.tsx,.js,.jsx"
--copy-files`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c6598f5a-8afa-4df1-8498-e73eaf4c89e7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c6598f5a-8afa-4df1-8498-e73eaf4c89e7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c6598f5a-8afa-4df1-8498-e73eaf4c89e7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c6598f5a-8afa-4df1-8498-e73eaf4c89e7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c6598f5a-8afa-4df1-8498-e73eaf4c89e7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9436732d-6e84-4d40-9974-f2fff00b2cd6 -->
[](9436732d-6e84-4d40-9974-f2fff00b2cd6)
##########
superset-frontend/scripts/build.js:
##########
@@ -105,35 +77,19 @@ function getPackages(packagePattern, tsOnly = false) {
let scope = getPackages(glob);
-if (shouldLint) {
- run(`npm run eslint -- . --fix {packages,plugins}/${scope}/{src,test}`);
-}
-
-if (shouldCleanup) {
- // these modules will be installed by `npm link` but not useful for actual
build
- const dirtyModules = 'node_modules/@types/react,node_modules/@superset-ui';
- const cachePath =
`./node_modules/${scope}/{lib,esm,tsconfig.tsbuildinfo,${dirtyModules}}`;
- console.log(`\n>> Cleaning up ${cachePath}`);
- sync(cachePath);
-}
+console.log('--- Run babel --------');
+const babelCommand = `lerna exec --stream --concurrency 10 --scope ${scope}
+ -- babel ${BABEL_CONFIG} src --extensions ".ts,.tsx,.js,.jsx"
--copy-files`;
+run(`${babelCommand} --out-dir lib`);
-if (shouldRunBabel) {
- console.log('--- Run babel --------');
- const babelCommand = `lerna exec --stream --concurrency 10 --scope ${scope}
- -- babel ${BABEL_CONFIG} src --extensions ".ts,.tsx,.js,.jsx"
--copy-files`;
- run(`${babelCommand} --out-dir lib`);
+console.log('--- Run babel esm ---');
+// run again with
+run(`${babelCommand} --out-dir esm`, {
+ env: { ...process.env, NODE_ENV: 'production', BABEL_OUTPUT: 'esm' },
+});
- console.log('--- Run babel esm ---');
- // run again with
- run(`${babelCommand} --out-dir esm`, {
- env: { ...process.env, NODE_ENV: 'production', BABEL_OUTPUT: 'esm' },
- });
-}
-
-if (shouldRunTyping) {
- console.log('--- Run tsc ---');
- // only run tsc for packages with ts files
- scope = getPackages(glob, true);
- run(`lerna exec --stream --concurrency 3 --scope ${scope} \
- -- ../../scripts/tsc.sh --build`);
-}
+console.log('--- Run tsc ---');
+// only run tsc for packages with ts files
+scope = getPackages(glob, true);
+run(`lerna exec --stream --concurrency 3 --scope ${scope} \
+ -- ../../scripts/tsc.sh --build`);
Review Comment:
### Suboptimal TypeScript Compilation Concurrency <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
TypeScript compilation is limited to a fixed concurrency of 3, which may not
be optimal for all systems.
###### Why this matters
Lower concurrency than necessary on powerful systems will increase build
times. Higher concurrency than available CPU cores can cause system slowdown.
###### Suggested change ∙ *Feature Preview*
Adjust TypeScript compilation concurrency based on available CPU cores:
```js
const tscConcurrency = Math.max(1, Math.min(cpuCount - 1, 3));
run(`lerna exec --stream --concurrency ${tscConcurrency} --scope ${scope} \
-- ../../scripts/tsc.sh --build`);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a5bac78-c3fe-41d6-96d0-1aa0fa3a0cdc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a5bac78-c3fe-41d6-96d0-1aa0fa3a0cdc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a5bac78-c3fe-41d6-96d0-1aa0fa3a0cdc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a5bac78-c3fe-41d6-96d0-1aa0fa3a0cdc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5a5bac78-c3fe-41d6-96d0-1aa0fa3a0cdc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5557235b-6f50-4ddb-845b-e8f8926cdb34 -->
[](5557235b-6f50-4ddb-845b-e8f8926cdb34)
##########
superset-frontend/scripts/build.js:
##########
@@ -25,39 +25,11 @@
*/
process.env.PATH = `./node_modules/.bin:${process.env.PATH}`;
-const { sync } = require('rimraf');
const { spawnSync } = require('child_process');
const fastGlob = require('fast-glob');
-const { argv } = require('yargs')
- .option('lint', {
- describe: 'whether to run ESLint',
- type: 'boolean',
- // lint is slow, so not turning it on by default
- default: false,
- })
- .option('babel', {
- describe: 'Whether to run Babel',
- type: 'boolean',
- default: true,
- })
- .option('clean', {
- describe: 'Whether to clean cache',
- type: 'boolean',
- default: false,
- })
- .option('type', {
- describe: 'Whether to run tsc',
- type: 'boolean',
- default: true,
- });
+const { argv } = require('yargs');
-const {
- _: globs,
- lint: shouldLint,
- babel: shouldRunBabel,
- clean: shouldCleanup,
- type: shouldRunTyping,
-} = argv;
+const { _: globs } = argv;
Review Comment:
### Missing Command Line Argument Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The command line arguments are destructured from argv without any validation
or default values, which could lead to runtime errors if invalid arguments are
provided.
###### Why this matters
If invalid command line arguments are provided, the build script may fail
unexpectedly or produce incorrect build artifacts.
###### Suggested change ∙ *Feature Preview*
Add argument validation and provide default values:
```javascript
const { _: globs = [] } = argv;
if (globs.some(g => typeof g !== 'string')) {
console.error('Invalid argument type provided');
process.exit(1);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d2779fe-6bb2-44ae-8c81-d7ee603c3f6d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d2779fe-6bb2-44ae-8c81-d7ee603c3f6d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d2779fe-6bb2-44ae-8c81-d7ee603c3f6d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d2779fe-6bb2-44ae-8c81-d7ee603c3f6d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3d2779fe-6bb2-44ae-8c81-d7ee603c3f6d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7a945f15-252c-4e69-906b-4415e6fc7b99 -->
[](7a945f15-252c-4e69-906b-4415e6fc7b99)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
