refactor(build): remove micro-utility dependencies - #7095
Open
43081j wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds local Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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
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. |
@netlify/build
@netlify/build-info
@netlify/cache-utils
@netlify/config
@netlify/edge-bundler
@netlify/functions-utils
@netlify/git-utils
@netlify/headers-parser
@netlify/api
@netlify/nock-udp
@netlify/opentelemetry-sdk-setup
@netlify/opentelemetry-utils
@netlify/redirect-parser
@netlify/run-utils
@netlify/zip-it-and-ship-it
commit: |
serhalp
reviewed
Jul 22, 2026
serhalp
approved these changes
Aug 2, 2026
serhalp
left a comment
serhalp
left a comment
Member
There was a problem hiding this comment.
LGTM, thank you!
📝 I think this one merits some manual QA
Comment on lines
-81
to
-104
| "ansi-escapes": "^7.0.0", | ||
| "ansis": "^4.1.0", | ||
| "clean-stack": "^5.0.0", | ||
| "execa": "^8.0.0", | ||
| "fast-string-width": "^3.0.2", | ||
| "fdir": "^6.0.1", | ||
| "figures": "^6.0.0", | ||
| "filter-obj": "^6.0.0", | ||
| "hot-shots": "11.4.0", | ||
| "ignore": "^7.0.0", | ||
| "indent-string": "^5.0.0", | ||
| "is-plain-obj": "^4.0.0", | ||
| "keep-func-props": "^6.0.0", | ||
| "log-process-errors": "^11.0.0", | ||
| "memoize-one": "^6.0.0", | ||
| "minimatch": "^10.2.4", | ||
| "os-name": "^6.0.0", | ||
| "p-event": "^6.0.0", | ||
| "p-filter": "^4.0.0", | ||
| "p-locate": "^6.0.0", | ||
| "p-map": "^7.0.0", | ||
| "p-reduce": "^3.0.0", | ||
| "package-directory": "^8.0.0", | ||
| "path-exists": "^5.0.0", |
| '@typescript-eslint/no-unsafe-return': 'off', | ||
| }, | ||
| }, | ||
| { |
Member
There was a problem hiding this comment.
This file was some gross one-time tech debt in order to get upgraded to eslint 9. We'd like to avoid adding anything to it. Is that possible?
Contributor
Author
There was a problem hiding this comment.
afaict its actually because of eslint-plugin-import not being able to resolve some things.
in #7044 i remove most of this
5 tasks
This was referenced Aug 4, 2026
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