Mz/bundle test cover #1146
Mz/bundle test cover #1146
Conversation
…es in husky commit check
| @@ -25,17 +25,12 @@ jobs: | |||
| registry-url: 'https://registry.npmjs.org' | |||
| cache: yarn | |||
| - uses: salesforcecli/github-workflows/.github/actions/yarnInstallWithRetries@main | |||
| - name: Install esbuild Dependencies | |||
There was a problem hiding this comment.
I specify these dev dependencies in package.json, so I removed this step in the workflow.
| - name: Update for Bundling | ||
| run: | | ||
| node scripts/updateForBundling.js | ||
| - name: Generate Bundle | ||
| - name: Generate Bundle and Check if there is any dependency that needs to be externalized and is not in the whitelist. |
There was a problem hiding this comment.
I used this script to bundle to detect if there is any dependencies that should be externalized.
| @@ -2,3 +2,4 @@ | |||
| . "$(dirname "$0")/_/husky.sh" | |||
|
|
|||
| yarn lint && yarn pretty-quick --staged | |||
| node ./scripts/scanTs.js | |||
There was a problem hiding this comment.
I added this extra step to scan all ts artifacts to list usages of fs.read*
| @@ -19,6 +19,8 @@ jobs: | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
| - uses: actions/setup-node@v4 | |||
| with: | |||
There was a problem hiding this comment.
I updated the node version to the latest because originally it uses node v18 which is not compatible with the dev dependency, npm-dts, which I just added
There was a problem hiding this comment.
What is the minimum supported node version for sfdx-core? Is there a way to not change nodejs version?
| @@ -19,6 +19,8 @@ jobs: | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
| - uses: actions/setup-node@v4 | |||
| with: | |||
There was a problem hiding this comment.
What is the minimum supported node version for sfdx-core? Is there a way to not change nodejs version?
| // Combine stdout and stderr to check the entire output | ||
| const output = `${stdout}\n${stderr}`; | ||
| // Check if the output contains the error string of esbuild | ||
| if (output.includes('[require-resolve-not-external]')) { |
There was a problem hiding this comment.
what is the error output like, would this statement be false if there were multiple errors in that array? you could just check for require-resolve-not-external?
There was a problem hiding this comment.
The output from bundling process is like this
So each error with details will list in a row.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| // Function to update package.json |
There was a problem hiding this comment.
could this comment be updated to why we need to update the pjson? - same for all similar jsdoc-style comments
scripts/bundlingUtils.js
Outdated
| const packageJson = JSON.parse(data); | ||
|
|
||
| // Update package name if necessary | ||
| if (packageJson.name && packageJson.name === '@salesforce/core') { |
There was a problem hiding this comment.
optional accessor? packageJson?.name===?
scripts/bundlingUtils.js
Outdated
|
|
||
| // Function to update logger.ts | ||
| function updateLoggerTs() { | ||
| const loggerPath = './src/logger/logger.ts'; |
There was a problem hiding this comment.
can this and checkTransformStreamPath be combined? they're doing a lot of the same? at least share some of the variables
What does this PR do?
This PR adds several checks for bundling to prevent potential changes from breaking the process, including:
i. report an error when detecting dynamic imports. —supported:dynamic-import=false and
ii. set log-override as error for dynamic imports
i. add a check in husky whenever there is a new commit to send warnings of every fs.read* usage in the codebase
i. check during husky commit hook
ii. report an error if the target reference does not exist in updateForBundling.js
iii. add explanation to where transformStream is referenced to raise developers’ caution
iv. during bundling, if const searchString = /${process.cwd()}${require("path").sep}tmp-lib/g; is not detected in the bundled file, an error should be raised and let Mingxuan Zhang or others from IDEx team know to figure it out.
What issues does this PR fix or reference?
@W-16304459@