deps: Update vurnable packages #124
Conversation
📝 WalkthroughWalkthroughThis PR updates test and build dependencies across multiple files. It removes matplotlib and ipympl from conda non-conda test requirements; removes ipykernel from venv smoke-test requirements; clears many packages from the ipywidgets7 venv test requirements; tightens and adds minimum/security versions (including numpy>=1.22.2, pillow, setuptools, tornado, requests, jupyter packages, ipython, urllib3, jupyterlab, etc.) in the ipywidgets8 venv test requirements; upgrades glob to ^9.3.5 and removes styled-components in package.json. Sequence Diagram(s)Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Artmann
force-pushed
the
update-vurnable-packages
branch
from
3832782 to
a5ed3fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
build/conda-nonconda-test-requirements.txt(0 hunks)build/venv-smoke-test-requirements.txt(0 hunks)build/venv-test-ipywidgets7-requirements.txt(0 hunks)build/venv-test-ipywidgets8-requirements.txt(2 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (3)
- build/conda-nonconda-test-requirements.txt
- build/venv-smoke-test-requirements.txt
- build/venv-test-ipywidgets7-requirements.txt
🔇 Additional comments (3)
build/venv-test-ipywidgets8-requirements.txt (2)
9-10: Clarify: Should matplotlib and ipympl remain in this env file?The PR description states matplotlib and ipympl were removed, and the enriched summary confirms removal from venv-test-ipywidgets7-requirements.txt and conda-nonconda-test-requirements.txt. However, lines 9–10 still include them here. Confirm this is intentional (ipywidgets8 env needs them) or if they should also be removed for consistency.
2-2: Security pinning looks solid; numpy constraint is reasonable.The addition of minimum-version constraints for transitive dependencies (lines 12–24) is good practice for supply-chain security and reproducibility. The numpy>=1.22.2 baseline is sensible for modern NumPy ecosystem support.
Also applies to: 12-24
package.json (1)
2174-2248: styled-components removal is safe.Search found no references to styled-components in the codebase, so removing it from dependencies won't break anything. This is clean dead code removal.
| "format-util": "^1.0.5", | ||
| "fs-extra": "^4.0.3", | ||
| "glob": "^7.1.2", | ||
| "glob": "^9.3.5", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify glob v9 compatibility; major version upgrade may have breaking changes.
This upgrade spans v7→v9, which typically includes breaking changes. Confirm that glob usage in the build pipeline (esbuild, build scripts, etc.) remains compatible and that tests pass.
Run build and test suite to confirm no regressions:
🏁 Script executed:
#!/bin/bash
# Verify the build and key tests pass with glob v9
npm run build 2>&1 | head -50
npm run test:unittests 2>&1 | head -50Length of output: 267
Now searching the codebase for glob usage:
Based on the research, glob v9 is a full rewrite with significant API and algorithm changes, shifting from callback-based to Promise-based APIs and changing exported function names and Glob class methods. Several options were removed (root, stat, fs) and behaviors changed (nounique, nosort no longer supported).
The original review's concern about breaking changes is valid and warrants careful verification before merging. However, without access to the full build output and actual glob usage patterns in the codebase (sandbox constraints prevent running the full build suite), definitive evidence of incompatibility cannot be established solely through code inspection.
**Ensure glob v9 API compatibility with existing codebase usages; verify build and test suite pass locally.**
<cite index="12-1,12-5">In version 9, glob was rewritten to only support an async version that returns promises or a sync version that returns the list of files directly</cite>—any callback-based invocations will silently break. Audit build scripts and source code for glob usage and confirm all references are updated to the Promise or sync API before merging.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for glob imports and usage in the codebase
rg -n "require\(['\"]glob" --type=js --type=ts
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Search for glob ES6 imports
rg -n "import.*from.*['\"]glob" --type=js --type=tsLength of output: 448
🏁 Script executed:
#!/bin/bash
# Search for glob references in build/config files
fd -e js -e ts -e json | xargs grep -l "glob" 2>/dev/null | head -20Length of output: 982
🏁 Script executed:
#!/bin/bash
# Check the actual build script configuration
find . -name "esbuild.js" -o -name "build.js" -o -name "*.config.js" | head -5Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look at the build-related package.json scripts more closely
rg -A5 "compile-release|updatePackageJsonForBundle" package.jsonLength of output: 1389
---
🏁 Script executed:
```shell
#!/bin/bash
# Check webTestReporter.js glob usage
cat build/webTestReporter.js | head -50
Length of output: 2216
🏁 Script executed:
#!/bin/bash
# Check webpack/common.js glob usage
cat build/webpack/common.js | head -50Length of output: 1735
🏁 Script executed:
#!/bin/bash
# Check gulpfile.js glob usage
cat gulpfile.js | head -100Length of output: 3512
🏁 Script executed:
#!/bin/bash
# Check fileSystem.node.ts glob usage
cat src/platform/common/platform/fileSystem.node.ts | head -50Length of output: 1870
🏁 Script executed:
#!/bin/bash
# Check telemetryGenerator.node.ts glob usage
cat src/telemetryGenerator.node.ts | head -50Length of output: 1784
🏁 Script executed:
#!/bin/bash
# Search for promisify(glob) or callback patterns
rg -n "promisify.*glob|glob\(" src/ --type=ts --type=js -A3 -B1Length of output: 2586
🏁 Script executed:
#!/bin/bash
# Find actual glob() usage in webTestReporter
rg -n "glob\(" build/webTestReporter.js -A2 -B2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for any glob callback patterns
rg -n "glob.*\(.*function|glob.*\(.*\(err," --type=js --type=tsLength of output: 277480
This glob v7→v9 upgrade will break the build—multiple locations use callback-based API that no longer exists in v9.
The codebase has:
- Callback-based
glob()calls (v9 removed):src/test/extension.serviceRegistry.vscode.test.ts:119,src/test/testRunner.ts:75,src/test/index.node.ts:195,src/telemetryGenerator.node.ts:775 promisify(glob)patterns (incompatible with v9 API):src/platform/common/platform/fileSystem.node.ts:25,src/test/interpreters/condaService.node.ts:68glob.sync({sync: true})calls (options changed):build/webpack/common.js:20,gulpfile.js:62
All must be rewritten to use v9's promise-based async API before this upgrade will work.
A few files weren't in use anymore.
Summary by CodeRabbit