Skip to content

Commit

Permalink
[chore] Fix TypeError in shouldSkipBundle During yarn build-for-devto…
Browse files Browse the repository at this point in the history
…ols-dev and yarn build-for-devtools-prod Commands (#29906)

## Summary

Fix bundle type filtering logic to correctly handle array input in
argv.type and use some with includes for accurate filtering. This
addresses a TypeError encountered during yarn build-for-devtools-prod
and yarn build-for-devtools-dev commands.

## Motivation

The current implementation of the `shouldSkipBundle` function in
`scripts/rollup/build.js` has two issues:

1. **Incorrect array handling in
`parseRequestedNames`([#29613](#29613
    
The function incorrectly wraps the `argv.type` value in an additional
array when it's already an array. This leads to a `TypeError:
names[i].split is not a function` when `parseRequestedNames` attempts to
split the nested array, as seen in this error message:
    
    ```
C:\Users\Administrator\Documents\새 폴더\react\scripts\rollup\build.js:76
        let splitNames = names[i].split(',');
                            ^
    TypeError: names[i].split is not a function
    ```
This PR fixes this by correctly handling both string and array inputs in
`argv.type`:
    
    ```diff
    - const requestedBundleTypes = argv.type
    -   ? parseRequestedNames([argv.type], 'uppercase')
+ const argvType = Array.isArray(argv.type) ? argv.type : [argv.type];
    + const requestedBundleTypes = argv.type
    +   ? parseRequestedNames(argvType, 'uppercase')
    ```
    
2. **Inaccurate filtering logic in
`shouldSkipBundle`([#29614](#29614
    
The function uses `Array.prototype.every` with `indexOf` to check if
**all** requested bundle types are missing in the current bundle type.
However, when multiple bundle types are requested (e.g., `['NODE',
'NODE_DEV']`), the function should skip a bundle only if **none** of the
requested types are present. The current implementation incorrectly
allows bundles that match any of the requested types.
    
    To illustrate, consider the following example output:
    
    ```
    requestedBundleTypes [ 'NODE', 'NODE_DEV' ]
    bundleType NODE_DEV
    isAskingForDifferentType false

    requestedBundleTypes [ 'NODE', 'NODE_DEV' ]
    bundleType NODE_PROD
    isAskingForDifferentType false  // Incorrect behavior
    ```
    
In this case, even though the bundle type is `NODE_PROD` and doesn't
include `NODE_DEV`, the bundle is not skipped due to the incorrect
logic.
    
This PR fixes this by replacing `every` with `some` and using `includes`
for a more accurate check:
    
    ```diff
    - const isAskingForDifferentType = requestedBundleTypes.every(
    -   requestedType => bundleType.indexOf(requestedType) === -1
    - );
    + const isAskingForDifferentType = requestedBundleTypes.some(
    +   requestedType => !bundleType.includes(requestedType)
    + );
    ```
    
This ensures that the bundle is skipped only if **none** of the
requested types are found in the `bundleType`.

This PR addresses both of these issues to ensure correct bundle type
filtering in various build scenarios.


## How did you test this change?

1. **Verification of `requestedBundleTypes` usage in
`shouldSkipBundle`:**

   * I manually tested the following scenarios:
* `yarn build`: Verified that `requestedBundleTypes` remains an empty
array, as expected.
* `yarn build-for-devtools`: Confirmed that `requestedBundleTypes` is
correctly set to `['NODE']`, as in the original implementation.
* `yarn build-for-devtools-dev`: This previously failed due to the
error. After the fix, I confirmed that `requestedBundleTypes` is now
correctly passed as `['NODE', 'NODE_DEV']`.

2. **Debugging of filtering logic in `shouldSkipBundle`:**

* I added the following logging statements to the `shouldSkipBundle`
function to observe its behavior during the build process:

     ```javascript
     console.log('requestedBundleTypes', requestedBundleTypes);
     console.log('bundleType', bundleType);
     console.log('isAskingForDifferentType', isAskingForDifferentType);
     ```

* By analyzing the log output, I confirmed that the filtering logic now
correctly identifies when a bundle should be skipped based on the
requested types. This allowed me to verify that the fix enables building
specific target bundles as intended.
  • Loading branch information
nakjun12 authored Jun 20, 2024
1 parent a555419 commit 395e2fc
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions scripts/rollup/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,11 @@ function parseRequestedNames(names, toCase) {
return result;
}

const argvType = Array.isArray(argv.type) ? argv.type : [argv.type];
const requestedBundleTypes = argv.type
? parseRequestedNames([argv.type], 'uppercase')
? parseRequestedNames(argvType, 'uppercase')
: [];

const requestedBundleNames = parseRequestedNames(argv._, 'lowercase');
const forcePrettyOutput = argv.pretty;
const isWatchMode = argv.watch;
Expand Down Expand Up @@ -528,8 +530,8 @@ function shouldSkipBundle(bundle, bundleType) {
return true;
}
if (requestedBundleTypes.length > 0) {
const isAskingForDifferentType = requestedBundleTypes.every(
requestedType => bundleType.indexOf(requestedType) === -1
const isAskingForDifferentType = requestedBundleTypes.some(
requestedType => !bundleType.includes(requestedType)
);
if (isAskingForDifferentType) {
return true;
Expand Down

0 comments on commit 395e2fc

Please sign in to comment.