refactor(build): remove micro-utility dependencies#7095
Open
43081j wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (46)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (36)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds local Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)packages/build/src/log/messages/mutations.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.14][ERROR]: unable to find a config; path packages/build/src/plugins/child/diff.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.31][ERROR]: unable to find a config; path packages/build/src/plugins/child/load.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.26][ERROR]: unable to find a config; path
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/build/src/utils/blobs.ts (1)
139-147: 🩺 Stability & Availability | 🟡 MinorUpdate the return type annotation for
datato remove theArrayBuffergeneric constraint.The function returns
Buffer<ArrayBuffer>, butreadFilereturnsBufferwhich is typed asBuffer<ArrayBufferLike>in@types/nodev22+. SinceArrayBufferLikeis a wider union (includingSharedArrayBuffer) and TypeScript generic parameters are invariant,Buffer<ArrayBufferLike>is not assignable to the narrowerBuffer<ArrayBuffer>.Update the return type to just
Bufferto allow the inferred type fromreadFile.Current code
```typescript ): Promise<{ data: Buffer; metadata: Record }> => { ```Change to:
): Promise<{ data: Buffer; metadata: Record<string, string> }> => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/build/src/utils/blobs.ts` around lines 139 - 147, The return type on the blob-reading helper is too narrow because it declares data as Buffer<ArrayBuffer>, which conflicts with the type returned by readFile in modern `@types/node`. Update the Promise return annotation in the blob utility function that reads contentPath and metadataPath so data is just Buffer, keeping metadata unchanged, and let the inferred type from readFile flow through without the ArrayBuffer generic constraint.
🧹 Nitpick comments (1)
packages/build/src/plugins/child/diff.js (1)
5-6: 🎯 Functional Correctness | 🔵 Trivial
isPlainObjectsemantics diverge fromis-plain-objbut are safe for this contextThe internal helper omits
Symbol.toStringTagandSymbol.iteratorchecks found inis-plain-objv4. This is acceptable:netlifyConfigis derived from JSON/YAML (which lack symbols), and the validation paths handle standard POJOs. The simplified logic reduces dependencies without impacting correctness for these inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/build/src/plugins/child/diff.js` around lines 5 - 6, The concern is the simplified isPlainObject behavior differs from is-plain-obj, but this path is safe because diff.js only validates netlifyConfig coming from JSON/YAML inputs. Keep using the internal isPlainObject helper in the child diff plugin and do not replace it with the external package; ensure the validation logic continues to rely on standard POJO checks only, since the existing netlifyConfig handling already covers the intended inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/build/src/plugins_core/blobs_upload/index.ts`:
- Around line 60-67: The shared-iterator worker loop in blobs_upload/index.ts
keeps pulling items from queue even after one upload fails, because Promise.all
rejects but the other async workers continue running. Update the worker logic
around the queue consumption and blobStore.set path so that the first caught
error flips a shared failure flag and prevents any further iterations from
reading new entries; reference the Promise.all worker block and the queue
iterator to gate new pulls once an error is seen.
In `@packages/build/src/plugins_core/dev_blobs_upload/index.ts`:
- Around line 63-73: The dev blob upload worker pool can keep processing after
one worker fails, because the shared queue iterator in dev_blobs_upload/index.ts
is still consumed by other async workers spawned in the Promise.all block.
Update the worker loop around the queue iteration in the upload routine to use a
shared stop flag that is set on first rejection, and check it before pulling the
next item from the queue so no further blobs are uploaded after a failure. Use
the existing worker-pool logic in this block (the queue, Promise.all, and the
per-item upload loop) as the place to add the early-exit behavior.
In `@packages/build/src/plugins/ipc.js`:
- Around line 53-58: Add an error listener to getEventFromChild so child_process
errors are handled alongside message and exit. Update the cleanup logic and
event wiring in getEventFromChild to register an error handler that rejects or
otherwise settles the promise, and make sure it is removed in cleanup with the
existing onMessage and onExit listeners.
---
Outside diff comments:
In `@packages/build/src/utils/blobs.ts`:
- Around line 139-147: The return type on the blob-reading helper is too narrow
because it declares data as Buffer<ArrayBuffer>, which conflicts with the type
returned by readFile in modern `@types/node`. Update the Promise return annotation
in the blob utility function that reads contentPath and metadataPath so data is
just Buffer, keeping metadata unchanged, and let the inferred type from readFile
flow through without the ArrayBuffer generic constraint.
---
Nitpick comments:
In `@packages/build/src/plugins/child/diff.js`:
- Around line 5-6: The concern is the simplified isPlainObject behavior differs
from is-plain-obj, but this path is safe because diff.js only validates
netlifyConfig coming from JSON/YAML inputs. Keep using the internal
isPlainObject helper in the child diff plugin and do not replace it with the
external package; ensure the validation logic continues to rely on standard POJO
checks only, since the existing netlifyConfig handling already covers the
intended inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c42c6848-b4b7-49b6-83fd-d2f9dedf84d5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (46)
eslint_temporary_suppressions.jspackages/build/package.jsonpackages/build/src/core/bin.jspackages/build/src/core/constants.tspackages/build/src/core/dry.jspackages/build/src/core/missing_side_file.jspackages/build/src/env/changes.jspackages/build/src/env/main.tspackages/build/src/env/metadata.jspackages/build/src/error/api.jspackages/build/src/error/handle.tspackages/build/src/error/parse/clean_stack.jspackages/build/src/install/main.jspackages/build/src/install/missing.jspackages/build/src/log/messages/core.tspackages/build/src/log/messages/mutations.jspackages/build/src/plugins/child/diff.jspackages/build/src/plugins/child/load.tspackages/build/src/plugins/child/status.tspackages/build/src/plugins/compatibility.tspackages/build/src/plugins/ipc.jspackages/build/src/plugins/list.tspackages/build/src/plugins/manifest/validate.jspackages/build/src/plugins_core/blobs_upload/index.tspackages/build/src/plugins_core/db_setup/migrations.tspackages/build/src/plugins_core/db_setup/utils.tspackages/build/src/plugins_core/deploy/buildbot_client.tspackages/build/src/plugins_core/dev_blobs_upload/index.tspackages/build/src/plugins_core/edge_functions/index.tspackages/build/src/plugins_core/frameworks_api/util.tspackages/build/src/plugins_core/functions/index.tspackages/build/src/plugins_core/functions_install/index.jspackages/build/src/steps/update_config.jspackages/build/src/utils/blobs.tspackages/build/src/utils/is_plain_object.tspackages/build/src/utils/omit.tspackages/build/src/utils/path_exists.tspackages/build/src/utils/remove_falsy.tspackages/build/tests/constants/fixtures/functions_src_missing/plugin.jspackages/build/tests/constants/fixtures/publish_missing/plugin.jspackages/build/tests/core/tests.jspackages/build/tests/edge_functions/fixtures/src_missing/plugin.jspackages/build/tests/edge_functions/tests.jspackages/build/tests/functions/tests.jspackages/build/tests/install/tests.jspackages/build/tests/mutate_save/tests.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
💤 Files with no reviewable changes (1)
- packages/build/package.json
Comment on lines
+63
to
+73
| const queue = blobsToUpload[Symbol.iterator]() | ||
| await Promise.all( | ||
| Array.from({ length: 10 }, async () => { | ||
| for (const { key, contentPath, metadataPath } of queue) { | ||
| if (debug && !quiet) { | ||
| log(logs, `- Uploading blob ${key}`, { indent: true }) | ||
| } | ||
| const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath) | ||
| await blobStore.set(key, new Blob([data]), { metadata }) | ||
| } | ||
| const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath) | ||
| // `data` is a `Buffer`, typed by @types/node as `Buffer<ArrayBufferLike>`, which TS | ||
| // rejects as a `BlobPart`; the runtime value is a valid blob part. | ||
| await blobStore.set(key, new Blob([data as BlobPart]), { metadata }) | ||
| }, | ||
| { concurrency: 10 }, | ||
| }), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop dev upload workers after the first failure.
This worker pool has the same failure mode: once one worker rejects, Promise.all returns, but other workers can continue pulling from queue and uploading in the background. Add the same stop flag before consuming more items.
Proposed fix
try {
+ let shouldStop = false
const queue = blobsToUpload[Symbol.iterator]()
await Promise.all(
Array.from({ length: 10 }, async () => {
- for (const { key, contentPath, metadataPath } of queue) {
- if (debug && !quiet) {
- log(logs, `- Uploading blob ${key}`, { indent: true })
- }
- const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath)
- await blobStore.set(key, new Blob([data]), { metadata })
+ while (!shouldStop) {
+ const next = queue.next()
+ if (next.done) {
+ return
+ }
+
+ const { key, contentPath, metadataPath } = next.value
+ try {
+ if (debug && !quiet) {
+ log(logs, `- Uploading blob ${key}`, { indent: true })
+ }
+ const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath)
+ await blobStore.set(key, new Blob([data]), { metadata })
+ } catch (error) {
+ shouldStop = true
+ throw error
+ }
}
}),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const queue = blobsToUpload[Symbol.iterator]() | |
| await Promise.all( | |
| Array.from({ length: 10 }, async () => { | |
| for (const { key, contentPath, metadataPath } of queue) { | |
| if (debug && !quiet) { | |
| log(logs, `- Uploading blob ${key}`, { indent: true }) | |
| } | |
| const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath) | |
| await blobStore.set(key, new Blob([data]), { metadata }) | |
| } | |
| const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath) | |
| // `data` is a `Buffer`, typed by @types/node as `Buffer<ArrayBufferLike>`, which TS | |
| // rejects as a `BlobPart`; the runtime value is a valid blob part. | |
| await blobStore.set(key, new Blob([data as BlobPart]), { metadata }) | |
| }, | |
| { concurrency: 10 }, | |
| }), | |
| let shouldStop = false | |
| const queue = blobsToUpload[Symbol.iterator]() | |
| await Promise.all( | |
| Array.from({ length: 10 }, async () => { | |
| while (!shouldStop) { | |
| const next = queue.next() | |
| if (next.done) { | |
| return | |
| } | |
| const { key, contentPath, metadataPath } = next.value | |
| try { | |
| if (debug && !quiet) { | |
| log(logs, `- Uploading blob ${key}`, { indent: true }) | |
| } | |
| const { data, metadata } = await getFileWithMetadata(key, contentPath, metadataPath) | |
| await blobStore.set(key, new Blob([data]), { metadata }) | |
| } catch (error) { | |
| shouldStop = true | |
| throw error | |
| } | |
| } | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/build/src/plugins_core/dev_blobs_upload/index.ts` around lines 63 -
73, The dev blob upload worker pool can keep processing after one worker fails,
because the shared queue iterator in dev_blobs_upload/index.ts is still consumed
by other async workers spawned in the Promise.all block. Update the worker loop
around the queue iteration in the upload routine to use a shared stop flag that
is set on first rejection, and check it before pulling the next item from the
queue so no further blobs are uploaded after a failure. Use the existing
worker-pool logic in this block (the queue, Promise.all, and the per-item upload
loop) as the place to add the early-exit behavior.
This removes 8 dependencies as follows: - `ansi-escapes` was only used for ANSI links, which we can inline instead - `clean-stack` was mostly made redundant by the fact we already do our own filtering beforehand. If we inline the only remaining case (node internals), it becomes one extra small RegExp - `filter-obj` can be achieved by a filtered `Object.entries` - `is-plain-obj` can be inlined as our own util - `p-event` can be replaced with `once` (from `node:events`) - `p-filter` was never used with a concurrency limit, so can be replaced by a regular `map` of async tasks - `p-locate` was never used with a concurrency limit, so can be replaced by a regular `for...of` - `p-map` can be replaced by a basic pool of `for...of` on a shared iterator Reasons: - Reducing the supply chain footprint is good for security - In most cases, we use a fraction of the provided functionality, so are wastefully pulling in a larger dependency than needed - If we keep doing this, size of the build package will reduce over time (baby steps but still progress)
Contributor
e18e dependency analysisNo dependency warnings found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This removes 9 dependencies as follows:
ansi-escapeswas only used for ANSI links, which we can inline insteadclean-stackwas mostly made redundant by the fact we already do our own filtering beforehand. If we inline the onlyremaining case (node internals), it becomes one extra small RegExp
filter-objcan be achieved by a filteredObject.entriesis-plain-objcan be inlined as our own utilp-eventcan be replaced withonce(fromnode:events)p-filterwas never used with a concurrency limit, so can be replaced by a regularmapof async tasksp-locatewas never used with a concurrency limit, so can be replaced by a regularfor...ofp-mapcan be replaced by a basic pool offor...ofon a shared iteratorpath-existscan be replaced byfs.accessReasons:
needed