chore: replace eslint-plugin-import with eslint-plugin-import-x#7044
Open
43081j wants to merge 6 commits into
Open
chore: replace eslint-plugin-import with eslint-plugin-import-x#704443081j wants to merge 6 commits into
43081j wants to merge 6 commits into
Conversation
This replaces `eslint-plugin-import` with the much lighter and more modern `eslint-plugin-import-x`. The gain here other than a smaller/faster plugin, is that modern package exports and typescript resolution is supported. **Also bumps typescript-eslint** which caught a bunch of minor warnings and errors.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR performs a comprehensive modernization of the build system, focusing on ESLint tooling, module import standardization, and code quality improvements. The changes migrate the import linting plugin from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
43081j
commented
May 5, 2026
| const results = ( | ||
| await Promise.all( | ||
| patterns.map((pattern) => { | ||
| patterns.map(async (pattern) => { |
Contributor
Author
There was a problem hiding this comment.
this is so the undefined return is still a Promise - basically to satisfy TSESLint. we can also just eslint-ignore the line
43081j
commented
May 5, 2026
| @@ -1,4 +1,5 @@ | |||
| import { NetlifyConfig, NetlifyPlugin } from '../index.js' | |||
| import { NetlifyPlugin } from '../types/netlify_plugin.js' | |||
| import { NetlifyConfig } from '../types/config/netlify_config.js' | |||
Contributor
Author
There was a problem hiding this comment.
these changed to remove a circular dependency, in that index.js imports this file
43081j
commented
May 5, 2026
| const { indent = false, color } = config | ||
| const stringA = indent ? indentString(string, INDENT_SIZE) : string | ||
| const stringB = String(stringA).replace(EMPTY_LINES_REGEXP, EMPTY_LINE) | ||
| const stringB = stringA.replace(EMPTY_LINES_REGEXP, EMPTY_LINE) |
Contributor
Author
There was a problem hiding this comment.
string is typed as being a string, so this should be fine. it is a slight behavioural change in that we were prev able to pass non-strings to this at runtime, though afaict we're the consumers of this
43081j
commented
May 5, 2026
| "exports": "./lib/index.js", | ||
| "exports": { | ||
| ".": "./lib/index.js", | ||
| "./lib/types.js": "./lib/types.js" |
Contributor
Author
There was a problem hiding this comment.
we import this ourselves in other monorepo packages. so this is correct but we could also just re-export the types from the index
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/config/src/case.ts (1)
2-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Buildconfiguration is silently discarded whenbuildis absent — confirmed behavioral regression.Commit fc92012 removed the
build = Buildfallback from the destructuring, introducing a critical regression. The function's documented purpose is to handle uppercase-capitalized properties, yet when a config provides onlyBuild(e.g., innetlify.tomlordefaultConfig), the value is now lost.When
buildis undefined,normalizeBuildCase(undefined)returns an empty object due to its= {}default, and the providedBuildvalue is permanently dropped. The TypeScript index signature[key: string]: unknownprevents compile-time detection when callers omit the lowercase key.Proposed fix:
export const normalizeConfigCase = function ({ Build, build, ...config }: { Build: Record<string, unknown> build: Record<string, unknown> [key: string]: unknown }): Record<string, unknown> { - const buildA = normalizeBuildCase(build) + const buildA = normalizeBuildCase(build ?? Build) return { ...config, build: buildA } }🤖 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/config/src/case.ts` around lines 2 - 13, normalizeConfigCase is dropping the uppercase Build value when lowercase build is absent; restore the original fallback by using the Build value for build before calling normalizeBuildCase (e.g., in normalizeConfigCase use destructuring with build = Build or compute const buildValue = build ?? Build and pass buildValue to normalizeBuildCase), so normalizeBuildCase receives the provided Build when build is undefined and the returned object spreads config with build: normalizeBuildCase(buildValue).packages/build/src/plugins_core/frameworks_api/index.ts (1)
58-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard optional logger on the error path
Line 65 calls
systemLog(...)unconditionally, butsystemLogis optional inCoreStepFunctionArgs. This can throw in thecatchpath and mask the originalloadConfigFileerror.💡 Proposed fix
-const coreStep: CoreStepFunction = async function ({ buildDir, netlifyConfig, packagePath, systemLog }) { +const coreStep: CoreStepFunction = async function ({ buildDir, netlifyConfig, packagePath, systemLog }) { await handleSkewProtection(buildDir, packagePath) let config: Partial<NetlifyConfig> | undefined try { config = await loadConfigFile(buildDir, packagePath) } catch (err) { - systemLog(`Failed to read Frameworks API: ${err.message}`) + systemLog?.(`Failed to read Frameworks API: ${(err as Error).message}`) throw new Error('An error occured while processing the platform configurarion defined by your framework') }🤖 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/frameworks_api/index.ts` around lines 58 - 66, The catch block in the coreStep (CoreStepFunction) calls the optional systemLog unguarded which can throw and hide the original loadConfigFile error; update the error path to check for existence of systemLog before invoking it (e.g., if (systemLog) systemLog(...)) and then rethrow or handle the original error accordingly so the original loadConfigFile failure is preserved; locate the catch around the await loadConfigFile(...) and modify the systemLog usage there.
🧹 Nitpick comments (3)
packages/zip-it-and-ship-it/src/runtimes/node/bundlers/zisi/list_imports.ts (1)
3-3: ⚡ Quick winUse named import to eliminate eslint-disable suppression
The
no-named-as-default-memberrule flags accessing named exports as properties on default imports, which is likely a mistake. Herepaperworkis a named export ofprecinct, confirming the suppress comment is only needed because the default import is used.Converting to a named import removes the suppression and aligns with the import style used elsewhere in the file (e.g.,
tmp-promiseon line 4):♻️ Proposed refactor
-import precinct from 'precinct' +import { paperwork } from 'precinct'- // eslint-disable-next-line import-x/no-named-as-default-member - return precinct.paperwork(path, { includeCore: false }) + return paperwork(path, { includeCore: false })Also applies to: 78-79
🤖 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/zip-it-and-ship-it/src/runtimes/node/bundlers/zisi/list_imports.ts` at line 3, Replace the default import of precinct with a named import for the paperwork export in list_imports.ts (i.e., import { paperwork } from 'precinct') and update any usages that access paperwork via the default (precinct.paperwork) to use the named symbol directly; then remove the now-unnecessary eslint-disable comment that was suppressing no-named-as-default-member (previously around the precinct.paperwork usage).packages/build/src/plugins/child/utils.ts (1)
24-35: ⚡ Quick winConsider adding a defensive default for
deployEnvVarsparameter.The function receives
deployEnvVarswithout a default value, and operations on it at lines 73 and 77 (.findIndex(),.push()) would crash if the value wereundefined. While the type system (CoreStepFunctionArgs) currently requires this parameter and all current callers provide it, a defensive fallback improves robustness against future changes.Suggested defensive patch
export const getUtils = function ({ event, constants: { FUNCTIONS_SRC, INTERNAL_FUNCTIONS_SRC, CACHE_DIR }, generatedFunctions = [], runState, - deployEnvVars, + deployEnvVars = [], }: { event: BuildEvent constants: NetlifyPluginConstants generatedFunctions?: ReturnValue['generatedFunctions'] runState: RunState - deployEnvVars: DeployEnvVarsData + deployEnvVars?: DeployEnvVarsData }): NetlifyPluginUtils {🤖 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/utils.ts` around lines 24 - 35, Add a defensive default for the deployEnvVars parameter in getUtils so methods like .findIndex() and .push() won't throw if undefined: in the getUtils parameter destructuring set deployEnvVars = [] (i.e. deployEnvVars: DeployEnvVarsData = []) and ensure any usages (the code that calls .findIndex() and .push()) operate on that array; update related type annotation if needed to keep TS happy.packages/js-client/package.json (1)
6-9: 💤 Low valueConsider adding an explicit
typesconditional to the new subpath export.Without it, TypeScript relies on automatically finding
./lib/types.d.tsvia.js→.d.tssubstitution, which only works withmoduleResolution: node16,nodenext, orbundler. Consumers with older resolution settings (node) cannot resolve the subpath at all. An explicittypesentry makes the contract unambiguous and is the recommended practice for published packages.♻️ Proposed improvement
"exports": { ".": "./lib/index.js", - "./lib/types.js": "./lib/types.js" + "./lib/types.js": { + "types": "./lib/types.d.ts", + "default": "./lib/types.js" + } },🤖 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/js-client/package.json` around lines 6 - 9, The "./lib/types.js" export should include an explicit types conditional so TypeScript can resolve the declaration for consumers using older moduleResolution modes: change the exports entry for "./lib/types.js" from a string to an export object that includes a "types" key pointing to "./lib/types.d.ts" (in addition to keeping any existing "import"/"require" or default fields if needed), ensuring the package.json "exports" map unambiguously exposes the type declarations.
🤖 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/error/parse/parse.ts`:
- Line 29: The code currently destructures location from errorInfo and passes it
into getLocationInfo/formatter paths that expect an object; guard against
location being undefined by checking if location is present before calling
getLocationInfo or any plugin/location formatter (e.g., in the code paths
referencing getLocationInfo, plugin, and tsConfig), and if absent call the
generic non-location formatter or return a safe default (empty message/object)
so the error reporting path never receives undefined and cannot crash.
In `@packages/config/src/mutations/config_prop_name.ts`:
- Line 11: The normalization logic for numeric keys is wrong because
Number.isInteger(key) will always be false for string-typed keys; update the
computation of normalizedKey (the line that uses Number.isInteger(key) and
DYNAMIC_OBJECT_PROPS) to coerce or validate the string key as a number before
checking integerness (e.g., use Number.isInteger(Number(key)) or a /^\d+$/ test)
so numeric string indices like '0' normalize to '*' when appropriate while
keeping the DYNAMIC_OBJECT_PROPS.has(propName) branch unchanged.
In `@packages/edge-bundler/node/formats/tarball.ts`:
- Line 215: finalizeTarballBundle currently awaits bundleDir.cleanup(), letting
cleanup rejections bubble and fail the overall build; wrap the call to
bundleDir.cleanup() in a try/catch inside finalizeTarballBundle/finalizeBundle
so that cleanup failures are caught, logged as a non-fatal warning (use the
existing logger, e.g., processLogger.warn or console.warn) and do not rethrow;
ensure the tarball write and hash steps (the code around the writeFile/hash at
line ~213) remain untouched and that the catch only demotes the error instead of
causing the function to reject.
In `@packages/redirect-parser/src/all.ts`:
- Around line 12-15: Restore runtime-safe defaults on the exported function
parseAllRedirects: ensure its parameters have safe default values (e.g.,
redirectsFiles defaults to [], netlifyConfigPath to an empty string,
configRedirects to {} and minimal to false) in the function signature or
immediately assign those defaults at the top of parseAllRedirects so callers
that omit or pass partial values won't hit runtime errors when using
redirectsFiles.map or accessing configRedirects.
In `@packages/zip-it-and-ship-it/src/runtimes/node/index.ts`:
- Line 97: The bundler output field finalMainFile can be undefined and later
used by zipNodeJs and extname, so ensure finalMainFile always has a safe
fallback string before downstream use (e.g., default to "index.js" or another
known entry). Update the assignment where finalMainFile is set (the object
property mainFile: finalMainFile) to coalesce with a fallback (finalMainFile ??
'index.js') or normalize it immediately after bundler output, and ensure any
subsequent calls in zipNodeJs and usages of extname(...) operate on this
guaranteed non-undefined string.
In `@packages/zip-it-and-ship-it/tests/main.test.ts`:
- Around line 1952-1955: The destructure of args sets { cwd, env: environment }
= {} but environment can be undefined and is later dereferenced
(environment.CARGO_TARGET_DIR) in the cargo branch; change the destructure to
provide a defensive default for env (e.g., { cwd, env: environment = {} } = {})
so environment is always an object, then keep the existing usage of
environment.CARGO_TARGET_DIR in the cargo branch (refer to the const binding of
rootCommand, args and the cargo branch that computes directory).
---
Outside diff comments:
In `@packages/build/src/plugins_core/frameworks_api/index.ts`:
- Around line 58-66: The catch block in the coreStep (CoreStepFunction) calls
the optional systemLog unguarded which can throw and hide the original
loadConfigFile error; update the error path to check for existence of systemLog
before invoking it (e.g., if (systemLog) systemLog(...)) and then rethrow or
handle the original error accordingly so the original loadConfigFile failure is
preserved; locate the catch around the await loadConfigFile(...) and modify the
systemLog usage there.
In `@packages/config/src/case.ts`:
- Around line 2-13: normalizeConfigCase is dropping the uppercase Build value
when lowercase build is absent; restore the original fallback by using the Build
value for build before calling normalizeBuildCase (e.g., in normalizeConfigCase
use destructuring with build = Build or compute const buildValue = build ??
Build and pass buildValue to normalizeBuildCase), so normalizeBuildCase receives
the provided Build when build is undefined and the returned object spreads
config with build: normalizeBuildCase(buildValue).
---
Nitpick comments:
In `@packages/build/src/plugins/child/utils.ts`:
- Around line 24-35: Add a defensive default for the deployEnvVars parameter in
getUtils so methods like .findIndex() and .push() won't throw if undefined: in
the getUtils parameter destructuring set deployEnvVars = [] (i.e. deployEnvVars:
DeployEnvVarsData = []) and ensure any usages (the code that calls .findIndex()
and .push()) operate on that array; update related type annotation if needed to
keep TS happy.
In `@packages/js-client/package.json`:
- Around line 6-9: The "./lib/types.js" export should include an explicit types
conditional so TypeScript can resolve the declaration for consumers using older
moduleResolution modes: change the exports entry for "./lib/types.js" from a
string to an export object that includes a "types" key pointing to
"./lib/types.d.ts" (in addition to keeping any existing "import"/"require" or
default fields if needed), ensuring the package.json "exports" map unambiguously
exposes the type declarations.
In `@packages/zip-it-and-ship-it/src/runtimes/node/bundlers/zisi/list_imports.ts`:
- Line 3: Replace the default import of precinct with a named import for the
paperwork export in list_imports.ts (i.e., import { paperwork } from 'precinct')
and update any usages that access paperwork via the default (precinct.paperwork)
to use the named symbol directly; then remove the now-unnecessary eslint-disable
comment that was suppressing no-named-as-default-member (previously around the
precinct.paperwork usage).
🪄 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: c05cc19e-61cd-4b68-94d0-ab20669e3d75
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (89)
eslint.config.jspackage.jsonpackages/build-info/src/file-system.tspackages/build-info/src/node/file-system.tspackages/build-info/src/workspaces/get-workspace-packages.tspackages/build-info/tests/mock-file-system.tspackages/build/src/core/main.tspackages/build/src/core/types.tspackages/build/src/core/user_node_version.jspackages/build/src/error/parse/parse.tspackages/build/src/error/types.tspackages/build/src/log/logger.tspackages/build/src/log/messages/compatibility.tspackages/build/src/plugins/child/utils.tspackages/build/src/plugins/compatibility.tspackages/build/src/plugins/expected_version.tspackages/build/src/plugins/internal.tspackages/build/src/plugins/list.tspackages/build/src/plugins/node_version.tspackages/build/src/plugins/options.tspackages/build/src/plugins/plugin_conditions.tspackages/build/src/plugins/spawn.tspackages/build/src/plugins_core/frameworks_api/index.tspackages/build/src/plugins_core/functions/zisi.tspackages/build/src/plugins_core/types.tspackages/build/src/status/validations.tspackages/build/src/steps/error.tspackages/build/src/steps/plugin.jspackages/build/src/time/main.tspackages/build/src/utils/semver.jspackages/build/tests/blobs_upload/tests.jspackages/build/tests/core/tests.jspackages/build/tests/edge_functions/tests.jspackages/build/tests/frameworks_api/tests.jspackages/build/tests/functions/tests.jspackages/build/tests/mutate_save/tests.jspackages/build/tests/plugins/tests.jspackages/build/tests/unit/logger/tests.jspackages/config/src/case.tspackages/config/src/env/main.tspackages/config/src/mutations/config_prop_name.tspackages/config/src/validate/validations.jspackages/edge-bundler/node/bridge.test.tspackages/edge-bundler/node/bridge.tspackages/edge-bundler/node/bundler.test.tspackages/edge-bundler/node/bundler.tspackages/edge-bundler/node/config.test.tspackages/edge-bundler/node/config.tspackages/edge-bundler/node/deploy_config.test.tspackages/edge-bundler/node/downloader.test.tspackages/edge-bundler/node/downloader.tspackages/edge-bundler/node/formats/tarball.tspackages/edge-bundler/node/import_map.test.tspackages/edge-bundler/node/main.test.tspackages/edge-bundler/node/npm_dependencies.tspackages/edge-bundler/node/package_json.test.tspackages/edge-bundler/node/server/server.test.tspackages/edge-bundler/node/stage_2.test.tspackages/edge-bundler/node/types.test.tspackages/edge-bundler/node/validation/manifest/index.tspackages/edge-bundler/test/integration/test.jspackages/edge-bundler/test/util.tspackages/functions-utils/src/main.tspackages/js-client/package.jsonpackages/opentelemetry-sdk-setup/src/bin.tspackages/redirect-parser/src/all.tspackages/run-utils/tests/main.test.tspackages/testing/src/server.tspackages/testing/src/tcp_server.tspackages/zip-it-and-ship-it/src/deps.d.tspackages/zip-it-and-ship-it/src/runtimes/index.tspackages/zip-it-and-ship-it/src/runtimes/node/bundlers/esbuild/bundler.tspackages/zip-it-and-ship-it/src/runtimes/node/bundlers/esbuild/index.tspackages/zip-it-and-ship-it/src/runtimes/node/bundlers/esbuild/plugin_native_modules.tspackages/zip-it-and-ship-it/src/runtimes/node/bundlers/esbuild/src_files.tspackages/zip-it-and-ship-it/src/runtimes/node/bundlers/zisi/list_imports.tspackages/zip-it-and-ship-it/src/runtimes/node/bundlers/zisi/nested.tspackages/zip-it-and-ship-it/src/runtimes/node/in_source_config/properties/schedule.tspackages/zip-it-and-ship-it/src/runtimes/node/index.tspackages/zip-it-and-ship-it/src/runtimes/node/parser/exports.tspackages/zip-it-and-ship-it/src/runtimes/node/parser/helpers.tspackages/zip-it-and-ship-it/src/runtimes/node/parser/imports.tspackages/zip-it-and-ship-it/src/runtimes/node/parser/index.tspackages/zip-it-and-ship-it/src/runtimes/node/utils/included_files.tspackages/zip-it-and-ship-it/src/runtimes/node/utils/node_version.tspackages/zip-it-and-ship-it/src/runtimes/rust/builder.tspackages/zip-it-and-ship-it/tests/main.test.tspackages/zip-it-and-ship-it/tests/telemetry.test.tspackages/zip-it-and-ship-it/tests/v2api.test.ts
💤 Files with no reviewable changes (1)
- packages/zip-it-and-ship-it/src/deps.d.ts
| // In those cases, we replace it by "*". | ||
| return keys.reduce((propName, key) => { | ||
| const normalizedKey = Number.isInteger(key) || DYNAMIC_OBJECT_PROPS.has(propName) ? '*' : String(key) | ||
| const normalizedKey = Number.isInteger(key) || DYNAMIC_OBJECT_PROPS.has(propName) ? '*' : key |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm current implementation and call sites.
rg -n -C2 '\bgetPropName\b|\bNumber\.isInteger\s*\(' --type ts
# 2) Check for tests that cover numeric-string keys.
rg -n -C2 "getPropName\\(\\[.*['\"][0-9]+['\"].*\\]\\)" --type ts
# 3) Reproduce JS semantics proving the check fails for string indices.
python - <<'PY'
print("Number.isInteger equivalent on '0' in JS semantics should be false; Python sanity for string-digit:")
key = '0'
is_array_index_current = False # mirrors Number.isInteger('0') result
is_array_index_fixed = key.isdigit()
print("current_check_for_'0':", is_array_index_current)
print("fixed_check_for_'0':", is_array_index_fixed)
PYRepository: netlify/build
Length of output: 2958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find test files for config_prop_name
echo "=== Looking for tests related to config_prop_name ==="
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "getPropName" 2>/dev/null || echo "No test matches found"
# Search for any file that imports getPropName
echo -e "\n=== Files importing getPropName ==="
rg -l "getPropName" --type ts
# Show context around getPropName function definition
echo -e "\n=== Full function definition ==="
cat -n packages/config/src/mutations/config_prop_name.tsRepository: netlify/build
Length of output: 1055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for test files in the config package
echo "=== Test files in config package ==="
find packages/config -name "*.test.ts" -o -name "*.spec.ts" 2>/dev/null
# Search for mutations tests
echo -e "\n=== Testing mutations directory ==="
ls -la packages/config/src/mutations/ 2>/dev/null || echo "No mutations directory"
# Find all test files in config
echo -e "\n=== All ts/js files in config/src that might have tests ==="
find packages/config -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) 2>/dev/nullRepository: netlify/build
Length of output: 495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at how getPropName is used in the codebase
echo "=== All usages of getPropName ==="
rg "getPropName" -A2 -B2 --type ts
# Check the DYNAMIC_OBJECT_PROPS constant
echo -e "\n=== DYNAMIC_OBJECT_PROPS definition ==="
rg "DYNAMIC_OBJECT_PROPS" -B2 -A5 --type tsRepository: netlify/build
Length of output: 1977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check apply.js and update.js for getPropName usage
echo "=== apply.js ==="
cat -n packages/config/src/mutations/apply.js | head -50
echo -e "\n=== Looking for getPropName in apply.js ==="
grep -n "getPropName\|propName" packages/config/src/mutations/apply.js || echo "Not found"
echo -e "\n=== update.js ==="
cat -n packages/config/src/mutations/update.js | head -50
echo -e "\n=== Looking for getPropName in update.js ==="
grep -n "getPropName" packages/config/src/mutations/update.js || echo "Not found"Repository: netlify/build
Length of output: 5046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check blueprints repo for getPropName usage
echo "=== Searching blueprints for getPropName ==="
rg "getPropName" --type ts --type js 2>/dev/null || echo "Not found in blueprints"
# Also check for config imports from config package
echo -e "\n=== Imports from `@netlify/build`:config ==="
rg "from.*@netlify/build.*config|from.*netlify/config" --type ts --type js 2>/dev/null || echo "Not found"Repository: netlify/blueprints
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where configMutations are created or populated
echo "=== Where configMutations are created/used ==="
rg "configMutations|keys.*:.*value.*:" --type ts --type js -B2 -A2 | head -100
# Look for MUTABLE_PROPS which defines valid mutation targets
echo -e "\n=== MUTABLE_PROPS definition ==="
rg "MUTABLE_PROPS" --type ts --type js -B2 -A10 | head -50
# Search for array index mutations specifically
echo -e "\n=== Array-like property modifications ==="
rg "\\[\s*['\"]\\d+['\"]\\s*\\]|functions\\.\\[|array.*index" --type ts --type js | head -20Repository: netlify/build
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the full MUTABLE_PROPS definition in apply.js
echo "=== Full MUTABLE_PROPS from apply.js ==="
grep -n "MUTABLE_PROPS" packages/config/src/mutations/apply.js
# Get more context around where mutations are applied
echo -e "\n=== Full apply.js file ==="
wc -l packages/config/src/mutations/apply.js
echo -e "\n=== Content of apply.js ==="
cat packages/config/src/mutations/apply.jsRepository: netlify/build
Length of output: 4027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check where configMutations originate or are created
echo "=== Search for where mutations with 'keys' are created ==="
rg "keys\s*:" -B2 -A2 --type ts --type js | head -60
# Look for any array property mutations
echo -e "\n=== Look for array-like config properties ==="
rg "\\[\s*['\"]\\d+['\"]\\s*\\]|\\[0\\]|\\[1\\]" --type ts --type js | head -20
# Check the setProp utility that's called with keys
echo -e "\n=== Check setProp implementation ==="
find . -name "set.js" -o -name "set.ts" 2>/dev/null | xargs cat 2>/dev/null || echo "setProp not found"Repository: netlify/build
Length of output: 8403
Number.isInteger(key) always returns false for string arguments—numeric indices won't normalize to '*'
Line 11 uses Number.isInteger(key) to detect array indices, but since key is typed as string, this check will always fail. For example, Number.isInteger('0') returns false, so numeric string indices like '0', '1' won't be normalized to '*' as intended by the comment.
The fix is correct:
Proposed fix
export const getPropName = function (keys: string[]) {
// Some properties are user-defined, i.e. we need to replace them with a "*" token
// Check if a property name is dynamic, such as `functions.{functionName}`, or
// is an array index.
// In those cases, we replace it by "*".
return keys.reduce((propName, key) => {
- const normalizedKey = Number.isInteger(key) || DYNAMIC_OBJECT_PROPS.has(propName) ? '*' : key
+ const isArrayIndex = /^\d+$/.test(key)
+ const normalizedKey = isArrayIndex || DYNAMIC_OBJECT_PROPS.has(propName) ? '*' : key
return propName === '' ? normalizedKey : `${propName}.${normalizedKey}`
}, '')
}📝 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 normalizedKey = Number.isInteger(key) || DYNAMIC_OBJECT_PROPS.has(propName) ? '*' : key | |
| export const getPropName = function (keys: string[]) { | |
| // Some properties are user-defined, i.e. we need to replace them with a "*" token | |
| // Check if a property name is dynamic, such as `functions.{functionName}`, or | |
| // is an array index. | |
| // In those cases, we replace it by "*". | |
| return keys.reduce((propName, key) => { | |
| const isArrayIndex = /^\d+$/.test(key) | |
| const normalizedKey = isArrayIndex || DYNAMIC_OBJECT_PROPS.has(propName) ? '*' : key | |
| return propName === '' ? normalizedKey : `${propName}.${normalizedKey}` | |
| }, '') | |
| } |
🤖 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/config/src/mutations/config_prop_name.ts` at line 11, The
normalization logic for numeric keys is wrong because Number.isInteger(key) will
always be false for string-typed keys; update the computation of normalizedKey
(the line that uses Number.isInteger(key) and DYNAMIC_OBJECT_PROPS) to coerce or
validate the string key as a number before checking integerness (e.g., use
Number.isInteger(Number(key)) or a /^\d+$/ test) so numeric string indices like
'0' normalize to '*' when appropriate while keeping the
DYNAMIC_OBJECT_PROPS.has(propName) branch unchanged.
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
1 participant
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 replaces
eslint-plugin-importwith the much lighter and more moderneslint-plugin-import-x.The gain here other than a smaller/faster plugin, is that modern package exports and typescript resolution is supported.
Also bumps typescript-eslint which caught a bunch of minor warnings and errors.
I'll leave comments explaining each notable change.
Notes
x !== undefined && x.footox?.foo